| Message ID | 1252935086-8856-1-git-send-email-yvugenfi@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
On 14.09.2009, at 15:31, Yan Vugenfirer wrote: > Signed-off-by: Yan Vugenfirer <yvugenfi@redhat.com> > > --- > hw/virtio-pci.c | 14 ++++++++++++-- > 1 files changed, 12 insertions(+), 2 deletions(-) > > diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c > index f812ab7..a0a22c4 100644 > --- a/hw/virtio-pci.c > +++ b/hw/virtio-pci.c > @@ -364,8 +364,17 @@ static void virtio_map(PCIDevice *pci_dev, int region_num, > static void virtio_write_config(PCIDevice *pci_dev, uint32_t address, > uint32_t val, int len) > { > + VirtIOPCIProxy *proxy = DO_UPCAST(VirtIOPCIProxy, pci_dev, pci_dev); > + > + if (PCI_COMMAND == address) { > + if (!(val & PCI_COMMAND_MASTER)) { > + proxy->vdev->status &= ~VIRTIO_CONFIG_S_DRIVER_OK; This part breaks PCI hotplug with Linux guests. What happens is the following: (qemu) pci_add auto nic model=virtio,vlan=0 - Virtio dev 1 -> write config (0) - Virtio dev 1 -> write config (0x3) - Virtio dev 1 -> set status explicitly to OK (qemu) pci_add auto storage file=/tmp/image.raw,if=virtio - Virtio dev 1 -> write config (0x3) -> Unset DRIVER_OK bit -> network device becomes unresponsive - Virtio dev 2 -> write config (0) - Virtio dev 2 -> write config (0x3) - Virtio dev 2 -> set status explicitly to OK Please either revert this patch or provide a proper fix. Alex
On Mon, Mar 01, 2010 at 12:14:43PM +0100, Alexander Graf wrote: > > On 14.09.2009, at 15:31, Yan Vugenfirer wrote: > > > Signed-off-by: Yan Vugenfirer <yvugenfi@redhat.com> > > > > --- > > hw/virtio-pci.c | 14 ++++++++++++-- > > 1 files changed, 12 insertions(+), 2 deletions(-) > > > > diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c > > index f812ab7..a0a22c4 100644 > > --- a/hw/virtio-pci.c > > +++ b/hw/virtio-pci.c > > @@ -364,8 +364,17 @@ static void virtio_map(PCIDevice *pci_dev, int region_num, > > static void virtio_write_config(PCIDevice *pci_dev, uint32_t address, > > uint32_t val, int len) > > { > > + VirtIOPCIProxy *proxy = DO_UPCAST(VirtIOPCIProxy, pci_dev, pci_dev); > > + > > + if (PCI_COMMAND == address) { > > + if (!(val & PCI_COMMAND_MASTER)) { > > + proxy->vdev->status &= ~VIRTIO_CONFIG_S_DRIVER_OK; > > This part breaks PCI hotplug with Linux guests. > > What happens is the following: Looks like something is broken even without this patch. > > (qemu) pci_add auto nic model=virtio,vlan=0 > - Virtio dev 1 -> write config (0) > - Virtio dev 1 -> write config (0x3) > - Virtio dev 1 -> set status explicitly to OK Why Linux doesn't enable bus mastering on this device? > > (qemu) pci_add auto storage file=/tmp/image.raw,if=virtio > - Virtio dev 1 -> write config (0x3) Why Linux touches unrelated device's config space? > -> Unset DRIVER_OK bit > -> network device becomes unresponsive > - Virtio dev 2 -> write config (0) > - Virtio dev 2 -> write config (0x3) > - Virtio dev 2 -> set status explicitly to OK > > > Please either revert this patch or provide a proper fix. > > Alex -- Gleb.
On Mon, Mar 01, 2010 at 01:26:37PM +0200, Gleb Natapov wrote: > On Mon, Mar 01, 2010 at 12:14:43PM +0100, Alexander Graf wrote: > > > > On 14.09.2009, at 15:31, Yan Vugenfirer wrote: > > > > > Signed-off-by: Yan Vugenfirer <yvugenfi@redhat.com> > > > > > > --- > > > hw/virtio-pci.c | 14 ++++++++++++-- > > > 1 files changed, 12 insertions(+), 2 deletions(-) > > > > > > diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c > > > index f812ab7..a0a22c4 100644 > > > --- a/hw/virtio-pci.c > > > +++ b/hw/virtio-pci.c > > > @@ -364,8 +364,17 @@ static void virtio_map(PCIDevice *pci_dev, int region_num, > > > static void virtio_write_config(PCIDevice *pci_dev, uint32_t address, > > > uint32_t val, int len) > > > { > > > + VirtIOPCIProxy *proxy = DO_UPCAST(VirtIOPCIProxy, pci_dev, pci_dev); > > > + > > > + if (PCI_COMMAND == address) { > > > + if (!(val & PCI_COMMAND_MASTER)) { > > > + proxy->vdev->status &= ~VIRTIO_CONFIG_S_DRIVER_OK; > > > > This part breaks PCI hotplug with Linux guests. > > > > What happens is the following: > Looks like something is broken even without this patch. > > > > > (qemu) pci_add auto nic model=virtio,vlan=0 > > - Virtio dev 1 -> write config (0) > > - Virtio dev 1 -> write config (0x3) > > - Virtio dev 1 -> set status explicitly to OK > Why Linux doesn't enable bus mastering on this device? I fixed this, and Rusty said he has applied the patch, but it seems the patch got lost later: http://lkml.org/lkml/2009/11/29/73 Alexander, could you please check whether applying this patch fixes hotplug for you? If yes I will queue it up for 2.6.34. > > > > (qemu) pci_add auto storage file=/tmp/image.raw,if=virtio > > - Virtio dev 1 -> write config (0x3) > Why Linux touches unrelated device's config space? > > > -> Unset DRIVER_OK bit > > -> network device becomes unresponsive > > - Virtio dev 2 -> write config (0) > > - Virtio dev 2 -> write config (0x3) > > - Virtio dev 2 -> set status explicitly to OK > > > > > > Please either revert this patch or provide a proper fix. > > > > Alex > > -- > Gleb.
On 02.03.2010, at 11:09, Michael S. Tsirkin wrote: > On Mon, Mar 01, 2010 at 01:26:37PM +0200, Gleb Natapov wrote: >> On Mon, Mar 01, 2010 at 12:14:43PM +0100, Alexander Graf wrote: >>> >>> On 14.09.2009, at 15:31, Yan Vugenfirer wrote: >>> >>>> Signed-off-by: Yan Vugenfirer <yvugenfi@redhat.com> >>>> >>>> --- >>>> hw/virtio-pci.c | 14 ++++++++++++-- >>>> 1 files changed, 12 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c >>>> index f812ab7..a0a22c4 100644 >>>> --- a/hw/virtio-pci.c >>>> +++ b/hw/virtio-pci.c >>>> @@ -364,8 +364,17 @@ static void virtio_map(PCIDevice *pci_dev, int region_num, >>>> static void virtio_write_config(PCIDevice *pci_dev, uint32_t address, >>>> uint32_t val, int len) >>>> { >>>> + VirtIOPCIProxy *proxy = DO_UPCAST(VirtIOPCIProxy, pci_dev, pci_dev); >>>> + >>>> + if (PCI_COMMAND == address) { >>>> + if (!(val & PCI_COMMAND_MASTER)) { >>>> + proxy->vdev->status &= ~VIRTIO_CONFIG_S_DRIVER_OK; >>> >>> This part breaks PCI hotplug with Linux guests. >>> >>> What happens is the following: >> Looks like something is broken even without this patch. >> >>> >>> (qemu) pci_add auto nic model=virtio,vlan=0 >>> - Virtio dev 1 -> write config (0) >>> - Virtio dev 1 -> write config (0x3) >>> - Virtio dev 1 -> set status explicitly to OK >> Why Linux doesn't enable bus mastering on this device? > > I fixed this, and Rusty said he has applied the patch, > but it seems the patch got lost later: > > http://lkml.org/lkml/2009/11/29/73 > > Alexander, could you please check whether applying > this patch fixes hotplug for you? > If yes I will queue it up for 2.6.34. So we're looking at a guest bug? It's still a qemu regression then. This configuration used to work with 0.10. There should at least be a feature bit from the guest saying "I'm good with bus mastering", so we can enable the check only for those. Alex
On Tue, Mar 02, 2010 at 12:03:15PM +0100, Alexander Graf wrote: > > On 02.03.2010, at 11:09, Michael S. Tsirkin wrote: > > > On Mon, Mar 01, 2010 at 01:26:37PM +0200, Gleb Natapov wrote: > >> On Mon, Mar 01, 2010 at 12:14:43PM +0100, Alexander Graf wrote: > >>> > >>> On 14.09.2009, at 15:31, Yan Vugenfirer wrote: > >>> > >>>> Signed-off-by: Yan Vugenfirer <yvugenfi@redhat.com> > >>>> > >>>> --- > >>>> hw/virtio-pci.c | 14 ++++++++++++-- > >>>> 1 files changed, 12 insertions(+), 2 deletions(-) > >>>> > >>>> diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c > >>>> index f812ab7..a0a22c4 100644 > >>>> --- a/hw/virtio-pci.c > >>>> +++ b/hw/virtio-pci.c > >>>> @@ -364,8 +364,17 @@ static void virtio_map(PCIDevice *pci_dev, int region_num, > >>>> static void virtio_write_config(PCIDevice *pci_dev, uint32_t address, > >>>> uint32_t val, int len) > >>>> { > >>>> + VirtIOPCIProxy *proxy = DO_UPCAST(VirtIOPCIProxy, pci_dev, pci_dev); > >>>> + > >>>> + if (PCI_COMMAND == address) { > >>>> + if (!(val & PCI_COMMAND_MASTER)) { > >>>> + proxy->vdev->status &= ~VIRTIO_CONFIG_S_DRIVER_OK; > >>> > >>> This part breaks PCI hotplug with Linux guests. > >>> > >>> What happens is the following: > >> Looks like something is broken even without this patch. > >> > >>> > >>> (qemu) pci_add auto nic model=virtio,vlan=0 > >>> - Virtio dev 1 -> write config (0) > >>> - Virtio dev 1 -> write config (0x3) > >>> - Virtio dev 1 -> set status explicitly to OK > >> Why Linux doesn't enable bus mastering on this device? > > > > I fixed this, and Rusty said he has applied the patch, > > but it seems the patch got lost later: > > > > http://lkml.org/lkml/2009/11/29/73 > > > > Alexander, could you please check whether applying > > this patch fixes hotplug for you? > > If yes I will queue it up for 2.6.34. > > So we're looking at a guest bug? Donnu. Does it work with the patch applied? > It's still a qemu regression then. This configuration used to work > with 0.10. There should at least be a feature bit from the guest > saying "I'm good with bus mastering", so we can enable the check only > for those. > Alex
On 02.03.2010, at 12:05, Michael S. Tsirkin wrote: > On Tue, Mar 02, 2010 at 12:03:15PM +0100, Alexander Graf wrote: >> >> On 02.03.2010, at 11:09, Michael S. Tsirkin wrote: >> >>> On Mon, Mar 01, 2010 at 01:26:37PM +0200, Gleb Natapov wrote: >>>> On Mon, Mar 01, 2010 at 12:14:43PM +0100, Alexander Graf wrote: >>>>> >>>>> On 14.09.2009, at 15:31, Yan Vugenfirer wrote: >>>>> >>>>>> Signed-off-by: Yan Vugenfirer <yvugenfi@redhat.com> >>>>>> >>>>>> --- >>>>>> hw/virtio-pci.c | 14 ++++++++++++-- >>>>>> 1 files changed, 12 insertions(+), 2 deletions(-) >>>>>> >>>>>> diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c >>>>>> index f812ab7..a0a22c4 100644 >>>>>> --- a/hw/virtio-pci.c >>>>>> +++ b/hw/virtio-pci.c >>>>>> @@ -364,8 +364,17 @@ static void virtio_map(PCIDevice *pci_dev, int region_num, >>>>>> static void virtio_write_config(PCIDevice *pci_dev, uint32_t address, >>>>>> uint32_t val, int len) >>>>>> { >>>>>> + VirtIOPCIProxy *proxy = DO_UPCAST(VirtIOPCIProxy, pci_dev, pci_dev); >>>>>> + >>>>>> + if (PCI_COMMAND == address) { >>>>>> + if (!(val & PCI_COMMAND_MASTER)) { >>>>>> + proxy->vdev->status &= ~VIRTIO_CONFIG_S_DRIVER_OK; >>>>> >>>>> This part breaks PCI hotplug with Linux guests. >>>>> >>>>> What happens is the following: >>>> Looks like something is broken even without this patch. >>>> >>>>> >>>>> (qemu) pci_add auto nic model=virtio,vlan=0 >>>>> - Virtio dev 1 -> write config (0) >>>>> - Virtio dev 1 -> write config (0x3) >>>>> - Virtio dev 1 -> set status explicitly to OK >>>> Why Linux doesn't enable bus mastering on this device? >>> >>> I fixed this, and Rusty said he has applied the patch, >>> but it seems the patch got lost later: >>> >>> http://lkml.org/lkml/2009/11/29/73 >>> >>> Alexander, could you please check whether applying >>> this patch fixes hotplug for you? >>> If yes I will queue it up for 2.6.34. >> >> So we're looking at a guest bug? > > Donnu. Does it work with the patch applied? Yes, it works with the patch applied. It's still missing a capability though :-). Alex
diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c index f812ab7..a0a22c4 100644 --- a/hw/virtio-pci.c +++ b/hw/virtio-pci.c @@ -364,8 +364,17 @@ static void virtio_map(PCIDevice *pci_dev, int region_num, static void virtio_write_config(PCIDevice *pci_dev, uint32_t address, uint32_t val, int len) { + VirtIOPCIProxy *proxy = DO_UPCAST(VirtIOPCIProxy, pci_dev, pci_dev); + + if (PCI_COMMAND == address) { + if (!(val & PCI_COMMAND_MASTER)) { + proxy->vdev->status &= ~VIRTIO_CONFIG_S_DRIVER_OK; + } + } + pci_default_write_config(pci_dev, address, val, len); - msix_write_config(pci_dev, address, val, len); + if(proxy->vdev->nvectors) + msix_write_config(pci_dev, address, val, len); } static const VirtIOBindings virtio_pci_bindings = { @@ -407,11 +416,12 @@ static void virtio_init_pci(VirtIOPCIProxy *proxy, VirtIODevice *vdev, msix_bar_size(&proxy->pci_dev), PCI_ADDRESS_SPACE_MEM, msix_mmio_map); - proxy->pci_dev.config_write = virtio_write_config; proxy->pci_dev.unregister = msix_uninit; } else vdev->nvectors = 0; + proxy->pci_dev.config_write = virtio_write_config; + size = VIRTIO_PCI_REGION_SIZE(&proxy->pci_dev) + vdev->config_len; if (size & (size-1)) size = 1 << qemu_fls(size);
Signed-off-by: Yan Vugenfirer <yvugenfi@redhat.com> --- hw/virtio-pci.c | 14 ++++++++++++-- 1 files changed, 12 insertions(+), 2 deletions(-)