| Message ID | 20100824064927.GB11495@kt |
|---|---|
| State | New |
| Headers | show |
Add Cc: mst@redhat.com. MAX_PCI_SLOTS should be in pci.h instead of qdev.h? And the name should be start with PCI_ prefix for consistency? Except that, the patches look okay. thanks, On Tue, Aug 24, 2010 at 02:49:27PM +0800, Ken CC wrote: > Define MAX_PCI_SLOTS as 0x1f, if pci addr provided from command line > is bigger than 0x1f, return error -EINVAL. > > 0x1f << 3 | 7 == 255 (PCIBUS_MAX_DEVICES - 1) > > Signed-off-by: Ken CC <ken.ccao@gmail.com> > --- > hw/qdev-properties.c | 2 ++ > hw/qdev.h | 3 +++ > 2 files changed, 5 insertions(+), 0 deletions(-) > > diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c > index 9219cd7..96d814c 100644 > --- a/hw/qdev-properties.c > +++ b/hw/qdev-properties.c > @@ -514,6 +514,8 @@ static int parse_pci_devfn(DeviceState *dev, Property *prop, const char *str) > return -EINVAL; > } > } > + if (slot > MAX_PCI_SLOTS) > + return -EINVAL; > if (str[n] != '\0') > return -EINVAL; > if (fn > 7) > diff --git a/hw/qdev.h b/hw/qdev.h > index 678f8b7..fcfe52e 100644 > --- a/hw/qdev.h > +++ b/hw/qdev.h > @@ -7,6 +7,9 @@ > #include "qemu-char.h" > #include "qemu-option.h" > > + > +#define MAX_PCI_SLOTS 0x1f > + > typedef struct Property Property; > > typedef struct PropertyInfo PropertyInfo; > >
On 08/24/2010 02:35 PM, Isaku Yamahata wrote: > Add Cc: mst@redhat.com. > > MAX_PCI_SLOTS should be in pci.h instead of qdev.h? > And the name should be start with PCI_ prefix for consistency? > > Except that, the patches look okay. > These aren't slots, are they? They are functions. There's a lot of slot/function confusion in qemu.
On 08/24/2010 03:07 PM, Isaku Yamahata wrote: > On Tue, Aug 24, 2010 at 02:42:18PM +0300, Avi Kivity wrote: >> On 08/24/2010 02:35 PM, Isaku Yamahata wrote: >>> Add Cc: mst@redhat.com. >>> >>> MAX_PCI_SLOTS should be in pci.h instead of qdev.h? >>> And the name should be start with PCI_ prefix for consistency? >>> >>> Except that, the patches look okay. >>> >> These aren't slots, are they? They are functions. > The function checks if given $slot.$fn (or $slot) is valid. > So it's slots. max 32. + assert(devfn< PCIBUS_MAX_DEVICES); Looks like we're comparing a function number to PCIBUS_MAX_DEVICES.
On Tue, Aug 24, 2010 at 02:42:18PM +0300, Avi Kivity wrote: > On 08/24/2010 02:35 PM, Isaku Yamahata wrote: >> Add Cc: mst@redhat.com. >> >> MAX_PCI_SLOTS should be in pci.h instead of qdev.h? >> And the name should be start with PCI_ prefix for consistency? >> >> Except that, the patches look okay. >> > > These aren't slots, are they? They are functions. The function checks if given $slot.$fn (or $slot) is valid. So it's slots. max 32. > There's a lot of slot/function confusion in qemu. Agreed.
On 08/24/2010 03:24 PM, Isaku Yamahata wrote: > On Tue, Aug 24, 2010 at 03:04:44PM +0300, Avi Kivity wrote: >> On 08/24/2010 03:07 PM, Isaku Yamahata wrote: >>> On Tue, Aug 24, 2010 at 02:42:18PM +0300, Avi Kivity wrote: >>>> On 08/24/2010 02:35 PM, Isaku Yamahata wrote: >>>>> Add Cc: mst@redhat.com. >>>>> >>>>> MAX_PCI_SLOTS should be in pci.h instead of qdev.h? >>>>> And the name should be start with PCI_ prefix for consistency? >>>>> >>>>> Except that, the patches look okay. >>>>> >>>> These aren't slots, are they? They are functions. >>> The function checks if given $slot.$fn (or $slot) is valid. >>> So it's slots. max 32. >> + assert(devfn< PCIBUS_MAX_DEVICES); >> >> >> Looks like we're comparing a function number to PCIBUS_MAX_DEVICES. > Ah, you're talking about 2/3. I talked about 3/3. > You're right. The name is misleading. > PCIBUS_MAX_FUNCTIONS? Or suggestions? Or assert(devfn / 8 < PCIBUS_MAX_DEVICES) (and change that to be 32).
On Tue, Aug 24, 2010 at 03:04:44PM +0300, Avi Kivity wrote: > On 08/24/2010 03:07 PM, Isaku Yamahata wrote: > >On Tue, Aug 24, 2010 at 02:42:18PM +0300, Avi Kivity wrote: > >> On 08/24/2010 02:35 PM, Isaku Yamahata wrote: > >>>Add Cc: mst@redhat.com. > >>> > >>>MAX_PCI_SLOTS should be in pci.h instead of qdev.h? > >>>And the name should be start with PCI_ prefix for consistency? > >>> > >>>Except that, the patches look okay. > >>> > >>These aren't slots, are they? They are functions. > >The function checks if given $slot.$fn (or $slot) is valid. > >So it's slots. max 32. > > + assert(devfn< PCIBUS_MAX_DEVICES); > > > Looks like we're comparing a function number to PCIBUS_MAX_DEVICES. > PCIBUS_MAX_DEVICES is the size of PCIBus.devices[], I have added it in the first patch at the defination of struct PCIBus, line 50 hw/pci.c. so i think the better name of the macro should be PCIBUS_MAX_FN, right? Ken > -- > error compiling committee.c: too many arguments to function > > -- > To unsubscribe from this list: send the line "unsubscribe kvm" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html --
On 08/24/2010 03:16 PM, Chen Cao wrote: > On Tue, Aug 24, 2010 at 03:04:44PM +0300, Avi Kivity wrote: >> On 08/24/2010 03:07 PM, Isaku Yamahata wrote: >>> On Tue, Aug 24, 2010 at 02:42:18PM +0300, Avi Kivity wrote: >>>> On 08/24/2010 02:35 PM, Isaku Yamahata wrote: >>>>> Add Cc: mst@redhat.com. >>>>> >>>>> MAX_PCI_SLOTS should be in pci.h instead of qdev.h? >>>>> And the name should be start with PCI_ prefix for consistency? >>>>> >>>>> Except that, the patches look okay. >>>>> >>>> These aren't slots, are they? They are functions. >>> The function checks if given $slot.$fn (or $slot) is valid. >>> So it's slots. max 32. >> + assert(devfn< PCIBUS_MAX_DEVICES); >> >> >> Looks like we're comparing a function number to PCIBUS_MAX_DEVICES. >> > PCIBUS_MAX_DEVICES is the size of PCIBus.devices[], I have added it in > the first patch at the defination of struct PCIBus, line 50 hw/pci.c. > so i think the better name of the macro should be PCIBUS_MAX_FN, > right? Or make it 32 and scale it by PCI_FUNCTIONS_PER_DEVICE. PCIBus.devices[] should be renamed to functions[] (later).
On Tue, Aug 24, 2010 at 03:04:44PM +0300, Avi Kivity wrote: > On 08/24/2010 03:07 PM, Isaku Yamahata wrote: >> On Tue, Aug 24, 2010 at 02:42:18PM +0300, Avi Kivity wrote: >>> On 08/24/2010 02:35 PM, Isaku Yamahata wrote: >>>> Add Cc: mst@redhat.com. >>>> >>>> MAX_PCI_SLOTS should be in pci.h instead of qdev.h? >>>> And the name should be start with PCI_ prefix for consistency? >>>> >>>> Except that, the patches look okay. >>>> >>> These aren't slots, are they? They are functions. >> The function checks if given $slot.$fn (or $slot) is valid. >> So it's slots. max 32. > > + assert(devfn< PCIBUS_MAX_DEVICES); > > > Looks like we're comparing a function number to PCIBUS_MAX_DEVICES. Ah, you're talking about 2/3. I talked about 3/3. You're right. The name is misleading. PCIBUS_MAX_FUNCTIONS? Or suggestions?
diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c index 9219cd7..96d814c 100644 --- a/hw/qdev-properties.c +++ b/hw/qdev-properties.c @@ -514,6 +514,8 @@ static int parse_pci_devfn(DeviceState *dev, Property *prop, const char *str) return -EINVAL; } } + if (slot > MAX_PCI_SLOTS) + return -EINVAL; if (str[n] != '\0') return -EINVAL; if (fn > 7) diff --git a/hw/qdev.h b/hw/qdev.h index 678f8b7..fcfe52e 100644 --- a/hw/qdev.h +++ b/hw/qdev.h @@ -7,6 +7,9 @@ #include "qemu-char.h" #include "qemu-option.h" + +#define MAX_PCI_SLOTS 0x1f + typedef struct Property Property; typedef struct PropertyInfo PropertyInfo;
Define MAX_PCI_SLOTS as 0x1f, if pci addr provided from command line is bigger than 0x1f, return error -EINVAL. 0x1f << 3 | 7 == 255 (PCIBUS_MAX_DEVICES - 1) Signed-off-by: Ken CC <ken.ccao@gmail.com> --- hw/qdev-properties.c | 2 ++ hw/qdev.h | 3 +++ 2 files changed, 5 insertions(+), 0 deletions(-)