Re: [Qemu-devel] [PATCH 3/3] Check pci slot number in parse_pci_devfn
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: m...@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). -- 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Qemu-devel] [PATCH 3/3] Check pci slot number in parse_pci_devfn
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: m...@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 majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Qemu-devel] [PATCH 3/3] Check pci slot number in parse_pci_devfn
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: m...@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). -- 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Qemu-devel] [PATCH 3/3] Check pci slot number in parse_pci_devfn
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: m...@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? -- yamahata -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Qemu-devel] [PATCH 3/3] Check pci slot number in parse_pci_devfn
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: m...@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. -- 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Qemu-devel] [PATCH 3/3] Check pci slot number in parse_pci_devfn
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: m...@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. -- yamahata -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Qemu-devel] [PATCH 3/3] Check pci slot number in parse_pci_devfn
On 08/24/2010 02:35 PM, Isaku Yamahata wrote: Add Cc: m...@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. -- 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Qemu-devel] [PATCH 3/3] Check pci slot number in parse_pci_devfn
Add Cc: m...@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 > --- > 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; > > -- yamahata -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
