Re: [PATCH] device-assignment: register a reset function
Am 15.11.2010 22:58, schrieb ext Jan Kiszka:
Am 15.11.2010 21:38, Alex Williamson wrote:
On Mon, 2010-11-15 at 13:08 +0100, Jan Kiszka wrote:
[Wrong list, it's not upstream yet. I'm migrating the thread to kvm.]
Am 15.11.2010 12:33, Bernhard Kohl wrote:
This is necessary because during reboot of a VM the assigned devices
continue DMA transfers which causes memory corruption.
Signed-off-by: Thomas Ostler
Signed-off-by: Bernhard Kohl
---
Sorry for for the long delay. Finally we added Alex' suggestions
and rebased the patch.
Thanks
Bernhard
---
hw/device-assignment.c | 12
1 files changed, 12 insertions(+), 0 deletions(-)
diff --git a/hw/device-assignment.c b/hw/device-assignment.c
index 5f5bde1..3f8de66 100644
--- a/hw/device-assignment.c
+++ b/hw/device-assignment.c
@@ -1434,6 +1434,17 @@ static void
assigned_dev_unregister_msix_mmio(AssignedDevice *dev)
dev->msix_table_page = NULL;
}
+static void reset_assigned_device(DeviceState *dev)
+{
+PCIDevice *d = DO_UPCAST(PCIDevice, qdev, dev);
+uint32_t conf;
+
+/* reset the bus master bit to avoid further DMA transfers */
+conf = assigned_dev_pci_read_config(d, PCI_COMMAND, 2);
+conf&= ~PCI_COMMAND_MASTER;
+assigned_dev_pci_write_config(d, PCI_COMMAND, conf, 2);
What about writing to /sys/bus/pci/devices/$DEVICE/reset? You probably
still need to put the command word into the reset state (ie. no RMW in
any case, just write 0), but the hardware should receive a reset as well
- if it is capable of doing a function-level reset, but we should at
least try.
libvirt doesn't currently give us write access to that file, so it'd
require changes up the stack too. We could accomplish the same by
deassigning and reassigning the device through KVM, but that seems error
prone. I'm not entirely convinced it's really necessary to go that far,
I expect there's some physical systems out there that don't reset the
device on a warm reset. In any case, I think doing this much is at
least a good start. Thanks,
OK, can be done on top of it - but should be done as most systems
perform a reset that is even stronger than pci_reset_function (I've seen
devices only recovering after warm reboot).
Still, I would suggest
assigned_dev_pci_write_config(d, PCI_COMMAND, 0, 2);
i.e. reset command word to specified reset state.
Yes, that's reasonable. I will resend the patch after testing.
Jan
--
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: [PATCH] device-assignment: register a reset function
Am 15.11.2010 21:38, Alex Williamson wrote:
> On Mon, 2010-11-15 at 13:08 +0100, Jan Kiszka wrote:
>> [Wrong list, it's not upstream yet. I'm migrating the thread to kvm.]
>>
>> Am 15.11.2010 12:33, Bernhard Kohl wrote:
>>> This is necessary because during reboot of a VM the assigned devices
>>> continue DMA transfers which causes memory corruption.
>>>
>>> Signed-off-by: Thomas Ostler
>>> Signed-off-by: Bernhard Kohl
>>> ---
>>> Sorry for for the long delay. Finally we added Alex' suggestions
>>> and rebased the patch.
>>>
>>> Thanks
>>> Bernhard
>>> ---
>>> hw/device-assignment.c | 12
>>> 1 files changed, 12 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/hw/device-assignment.c b/hw/device-assignment.c
>>> index 5f5bde1..3f8de66 100644
>>> --- a/hw/device-assignment.c
>>> +++ b/hw/device-assignment.c
>>> @@ -1434,6 +1434,17 @@ static void
>>> assigned_dev_unregister_msix_mmio(AssignedDevice *dev)
>>> dev->msix_table_page = NULL;
>>> }
>>>
>>> +static void reset_assigned_device(DeviceState *dev)
>>> +{
>>> +PCIDevice *d = DO_UPCAST(PCIDevice, qdev, dev);
>>> +uint32_t conf;
>>> +
>>> +/* reset the bus master bit to avoid further DMA transfers */
>>> +conf = assigned_dev_pci_read_config(d, PCI_COMMAND, 2);
>>> +conf &= ~PCI_COMMAND_MASTER;
>>> +assigned_dev_pci_write_config(d, PCI_COMMAND, conf, 2);
>>
>> What about writing to /sys/bus/pci/devices/$DEVICE/reset? You probably
>> still need to put the command word into the reset state (ie. no RMW in
>> any case, just write 0), but the hardware should receive a reset as well
>> - if it is capable of doing a function-level reset, but we should at
>> least try.
>
> libvirt doesn't currently give us write access to that file, so it'd
> require changes up the stack too. We could accomplish the same by
> deassigning and reassigning the device through KVM, but that seems error
> prone. I'm not entirely convinced it's really necessary to go that far,
> I expect there's some physical systems out there that don't reset the
> device on a warm reset. In any case, I think doing this much is at
> least a good start. Thanks,
OK, can be done on top of it - but should be done as most systems
perform a reset that is even stronger than pci_reset_function (I've seen
devices only recovering after warm reboot).
Still, I would suggest
assigned_dev_pci_write_config(d, PCI_COMMAND, 0, 2);
i.e. reset command word to specified reset state.
Jan
signature.asc
Description: OpenPGP digital signature
Re: [PATCH] device-assignment: register a reset function
On Mon, 2010-11-15 at 13:08 +0100, Jan Kiszka wrote:
> [Wrong list, it's not upstream yet. I'm migrating the thread to kvm.]
>
> Am 15.11.2010 12:33, Bernhard Kohl wrote:
> > This is necessary because during reboot of a VM the assigned devices
> > continue DMA transfers which causes memory corruption.
> >
> > Signed-off-by: Thomas Ostler
> > Signed-off-by: Bernhard Kohl
> > ---
> > Sorry for for the long delay. Finally we added Alex' suggestions
> > and rebased the patch.
> >
> > Thanks
> > Bernhard
> > ---
> > hw/device-assignment.c | 12
> > 1 files changed, 12 insertions(+), 0 deletions(-)
> >
> > diff --git a/hw/device-assignment.c b/hw/device-assignment.c
> > index 5f5bde1..3f8de66 100644
> > --- a/hw/device-assignment.c
> > +++ b/hw/device-assignment.c
> > @@ -1434,6 +1434,17 @@ static void
> > assigned_dev_unregister_msix_mmio(AssignedDevice *dev)
> > dev->msix_table_page = NULL;
> > }
> >
> > +static void reset_assigned_device(DeviceState *dev)
> > +{
> > +PCIDevice *d = DO_UPCAST(PCIDevice, qdev, dev);
> > +uint32_t conf;
> > +
> > +/* reset the bus master bit to avoid further DMA transfers */
> > +conf = assigned_dev_pci_read_config(d, PCI_COMMAND, 2);
> > +conf &= ~PCI_COMMAND_MASTER;
> > +assigned_dev_pci_write_config(d, PCI_COMMAND, conf, 2);
>
> What about writing to /sys/bus/pci/devices/$DEVICE/reset? You probably
> still need to put the command word into the reset state (ie. no RMW in
> any case, just write 0), but the hardware should receive a reset as well
> - if it is capable of doing a function-level reset, but we should at
> least try.
libvirt doesn't currently give us write access to that file, so it'd
require changes up the stack too. We could accomplish the same by
deassigning and reassigning the device through KVM, but that seems error
prone. I'm not entirely convinced it's really necessary to go that far,
I expect there's some physical systems out there that don't reset the
device on a warm reset. In any case, I think doing this much is at
least a good start. Thanks,
Alex
> > +}
> > +
> > static int assigned_initfn(struct PCIDevice *pci_dev)
> > {
> > AssignedDevice *dev = DO_UPCAST(AssignedDevice, dev, pci_dev);
> > @@ -1544,6 +1555,7 @@ static PCIDeviceInfo assign_info = {
> > .qdev.name= "pci-assign",
> > .qdev.desc= "pass through host pci devices to the guest",
> > .qdev.size= sizeof(AssignedDevice),
> > +.qdev.reset = reset_assigned_device,
> > .init = assigned_initfn,
> > .exit = assigned_exitfn,
> > .config_read = assigned_dev_pci_read_config,
>
> Jan
>
--
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: [PATCH] device-assignment: register a reset function
[Wrong list, it's not upstream yet. I'm migrating the thread to kvm.]
Am 15.11.2010 12:33, Bernhard Kohl wrote:
> This is necessary because during reboot of a VM the assigned devices
> continue DMA transfers which causes memory corruption.
>
> Signed-off-by: Thomas Ostler
> Signed-off-by: Bernhard Kohl
> ---
> Sorry for for the long delay. Finally we added Alex' suggestions
> and rebased the patch.
>
> Thanks
> Bernhard
> ---
> hw/device-assignment.c | 12
> 1 files changed, 12 insertions(+), 0 deletions(-)
>
> diff --git a/hw/device-assignment.c b/hw/device-assignment.c
> index 5f5bde1..3f8de66 100644
> --- a/hw/device-assignment.c
> +++ b/hw/device-assignment.c
> @@ -1434,6 +1434,17 @@ static void
> assigned_dev_unregister_msix_mmio(AssignedDevice *dev)
> dev->msix_table_page = NULL;
> }
>
> +static void reset_assigned_device(DeviceState *dev)
> +{
> +PCIDevice *d = DO_UPCAST(PCIDevice, qdev, dev);
> +uint32_t conf;
> +
> +/* reset the bus master bit to avoid further DMA transfers */
> +conf = assigned_dev_pci_read_config(d, PCI_COMMAND, 2);
> +conf &= ~PCI_COMMAND_MASTER;
> +assigned_dev_pci_write_config(d, PCI_COMMAND, conf, 2);
What about writing to /sys/bus/pci/devices/$DEVICE/reset? You probably
still need to put the command word into the reset state (ie. no RMW in
any case, just write 0), but the hardware should receive a reset as well
- if it is capable of doing a function-level reset, but we should at
least try.
> +}
> +
> static int assigned_initfn(struct PCIDevice *pci_dev)
> {
> AssignedDevice *dev = DO_UPCAST(AssignedDevice, dev, pci_dev);
> @@ -1544,6 +1555,7 @@ static PCIDeviceInfo assign_info = {
> .qdev.name= "pci-assign",
> .qdev.desc= "pass through host pci devices to the guest",
> .qdev.size= sizeof(AssignedDevice),
> +.qdev.reset = reset_assigned_device,
> .init = assigned_initfn,
> .exit = assigned_exitfn,
> .config_read = assigned_dev_pci_read_config,
Jan
--
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux
--
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: [PATCH] device-assignment: register a reset function
Am 17.09.2010 18:16, schrieb ext Alex Williamson:
On Fri, 2010-09-17 at 17:27 +0200, Bernhard Kohl wrote:
This is necessary because during reboot of a VM the assigned devices
continue DMA transfers which causes memory corruption.
Signed-off-by: Thomas Ostler
Signed-off-by: Bernhard Kohl
---
hw/device-assignment.c | 14 ++
1 files changed, 14 insertions(+), 0 deletions(-)
diff --git a/hw/device-assignment.c b/hw/device-assignment.c
index 87f7418..fb47813 100644
--- a/hw/device-assignment.c
+++ b/hw/device-assignment.c
@@ -1450,6 +1450,17 @@ static void
assigned_dev_unregister_msix_mmio(AssignedDevice *dev)
dev->msix_table_page = NULL;
}
+static void reset_assigned_device(void *opaque)
+{
+PCIDevice *d = (PCIDevice *)opaque;
+uint32_t conf;
+
+/* reset the bus master bit to avoid further DMA transfers */
+conf = assigned_dev_pci_read_config(d, PCI_COMMAND, 2);
+conf&= ~PCI_COMMAND_MASTER;
+assigned_dev_pci_write_config(d, PCI_COMMAND, conf, 2);
+}
+
static int assigned_initfn(struct PCIDevice *pci_dev)
{
AssignedDevice *dev = DO_UPCAST(AssignedDevice, dev, pci_dev);
@@ -1499,6 +1510,9 @@ static int assigned_initfn(struct PCIDevice *pci_dev)
if (r< 0)
goto assigned_out;
+/* register reset function for the device */
+qemu_register_reset(reset_assigned_device, pci_dev);
+
/* intercept MSI-X entry page in the MMIO */
if (dev->cap.available& ASSIGNED_DEVICE_CAP_MSIX)
if (assigned_dev_register_msix_mmio(dev))
Hmm, at a minimum, we need a qemu_unregister_reset() in the exitfn, but
upon further inspection, we should probably just do it the qdev way.
That would mean simply setting qdev.reset to reset_assigned_device() in
assign_info, then we can leave the registration/de-registration to qdev.
Does that work? Sorry I missed that the first time. Thanks,
Alex
OK, we will rework the patch for qdev.
This might take 2 weeks because of vacation.
Thanks
Bernhard
--
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: [PATCH] device-assignment: register a reset function
On Fri, 2010-09-17 at 17:27 +0200, Bernhard Kohl wrote:
> This is necessary because during reboot of a VM the assigned devices
> continue DMA transfers which causes memory corruption.
>
> Signed-off-by: Thomas Ostler
> Signed-off-by: Bernhard Kohl
> ---
> hw/device-assignment.c | 14 ++
> 1 files changed, 14 insertions(+), 0 deletions(-)
>
> diff --git a/hw/device-assignment.c b/hw/device-assignment.c
> index 87f7418..fb47813 100644
> --- a/hw/device-assignment.c
> +++ b/hw/device-assignment.c
> @@ -1450,6 +1450,17 @@ static void
> assigned_dev_unregister_msix_mmio(AssignedDevice *dev)
> dev->msix_table_page = NULL;
> }
>
> +static void reset_assigned_device(void *opaque)
> +{
> +PCIDevice *d = (PCIDevice *)opaque;
> +uint32_t conf;
> +
> +/* reset the bus master bit to avoid further DMA transfers */
> +conf = assigned_dev_pci_read_config(d, PCI_COMMAND, 2);
> +conf &= ~PCI_COMMAND_MASTER;
> +assigned_dev_pci_write_config(d, PCI_COMMAND, conf, 2);
> +}
> +
> static int assigned_initfn(struct PCIDevice *pci_dev)
> {
> AssignedDevice *dev = DO_UPCAST(AssignedDevice, dev, pci_dev);
> @@ -1499,6 +1510,9 @@ static int assigned_initfn(struct PCIDevice *pci_dev)
> if (r < 0)
> goto assigned_out;
>
> +/* register reset function for the device */
> +qemu_register_reset(reset_assigned_device, pci_dev);
> +
> /* intercept MSI-X entry page in the MMIO */
> if (dev->cap.available & ASSIGNED_DEVICE_CAP_MSIX)
> if (assigned_dev_register_msix_mmio(dev))
Hmm, at a minimum, we need a qemu_unregister_reset() in the exitfn, but
upon further inspection, we should probably just do it the qdev way.
That would mean simply setting qdev.reset to reset_assigned_device() in
assign_info, then we can leave the registration/de-registration to qdev.
Does that work? Sorry I missed that the first time. Thanks,
Alex
--
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
[PATCH] device-assignment: register a reset function
This is necessary because during reboot of a VM the assigned devices
continue DMA transfers which causes memory corruption.
Signed-off-by: Thomas Ostler
Signed-off-by: Bernhard Kohl
---
hw/device-assignment.c | 14 ++
1 files changed, 14 insertions(+), 0 deletions(-)
diff --git a/hw/device-assignment.c b/hw/device-assignment.c
index 87f7418..fb47813 100644
--- a/hw/device-assignment.c
+++ b/hw/device-assignment.c
@@ -1450,6 +1450,17 @@ static void
assigned_dev_unregister_msix_mmio(AssignedDevice *dev)
dev->msix_table_page = NULL;
}
+static void reset_assigned_device(void *opaque)
+{
+PCIDevice *d = (PCIDevice *)opaque;
+uint32_t conf;
+
+/* reset the bus master bit to avoid further DMA transfers */
+conf = assigned_dev_pci_read_config(d, PCI_COMMAND, 2);
+conf &= ~PCI_COMMAND_MASTER;
+assigned_dev_pci_write_config(d, PCI_COMMAND, conf, 2);
+}
+
static int assigned_initfn(struct PCIDevice *pci_dev)
{
AssignedDevice *dev = DO_UPCAST(AssignedDevice, dev, pci_dev);
@@ -1499,6 +1510,9 @@ static int assigned_initfn(struct PCIDevice *pci_dev)
if (r < 0)
goto assigned_out;
+/* register reset function for the device */
+qemu_register_reset(reset_assigned_device, pci_dev);
+
/* intercept MSI-X entry page in the MMIO */
if (dev->cap.available & ASSIGNED_DEVICE_CAP_MSIX)
if (assigned_dev_register_msix_mmio(dev))
--
1.7.2.2
--
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: [PATCH] device-assignment: register a reset function
Am 16.09.2010 17:48, schrieb ext Alex Williamson:
+static void reset_assigned_device(void *opaque)
> +{
> +PCIDevice *d = (PCIDevice *)opaque;
> +uint32_t conf;
> +
> +/* reset the bus master bit to avoid further DMA transfers */
> +conf = assigned_dev_pci_read_config(d, 0x04, 0x02);
> +conf&= ~0x04;
> +assigned_dev_pci_write_config(d, 0x04, conf, 0x02);
This should use defined macros, PCI_COMMAND& PCI_COMMAND_MASTER.
Otherwise seems ok. Thanks,
Alex
Thanks for the review. Will be fixed in v2.
Bernhard
--
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: [PATCH] device-assignment: register a reset function
On Tue, Sep 14, 2010 at 9:04 AM, Bernhard Kohl wrote:
> This is necessary because during reboot of a VM the assigned devices
> continue DMA transfers which causes memory corruption.
>
> Signed-off-by: Thomas Ostler
> Signed-off-by: Bernhard Kohl
> ---
> hw/device-assignment.c | 14 ++
> 1 files changed, 14 insertions(+), 0 deletions(-)
>
> diff --git a/hw/device-assignment.c b/hw/device-assignment.c
> index 87f7418..001aee8 100644
> --- a/hw/device-assignment.c
> +++ b/hw/device-assignment.c
> @@ -1450,6 +1450,17 @@ static void
> assigned_dev_unregister_msix_mmio(AssignedDevice *dev)
> dev->msix_table_page = NULL;
> }
>
> +static void reset_assigned_device(void *opaque)
> +{
> + PCIDevice *d = (PCIDevice *)opaque;
> + uint32_t conf;
> +
> + /* reset the bus master bit to avoid further DMA transfers */
> + conf = assigned_dev_pci_read_config(d, 0x04, 0x02);
> + conf &= ~0x04;
> + assigned_dev_pci_write_config(d, 0x04, conf, 0x02);
This should use defined macros, PCI_COMMAND & PCI_COMMAND_MASTER.
Otherwise seems ok. Thanks,
Alex
> +}
> +
> static int assigned_initfn(struct PCIDevice *pci_dev)
> {
> AssignedDevice *dev = DO_UPCAST(AssignedDevice, dev, pci_dev);
> @@ -1499,6 +1510,9 @@ static int assigned_initfn(struct PCIDevice *pci_dev)
> if (r < 0)
> goto assigned_out;
>
> + /* register reset function for the device */
> + qemu_register_reset(reset_assigned_device, pci_dev);
> +
> /* intercept MSI-X entry page in the MMIO */
> if (dev->cap.available & ASSIGNED_DEVICE_CAP_MSIX)
> if (assigned_dev_register_msix_mmio(dev))
> --
> 1.7.2.2
>
> --
> 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
[PATCH] device-assignment: register a reset function
This is necessary because during reboot of a VM the assigned devices
continue DMA transfers which causes memory corruption.
Signed-off-by: Thomas Ostler
Signed-off-by: Bernhard Kohl
---
hw/device-assignment.c | 14 ++
1 files changed, 14 insertions(+), 0 deletions(-)
diff --git a/hw/device-assignment.c b/hw/device-assignment.c
index 87f7418..001aee8 100644
--- a/hw/device-assignment.c
+++ b/hw/device-assignment.c
@@ -1450,6 +1450,17 @@ static void
assigned_dev_unregister_msix_mmio(AssignedDevice *dev)
dev->msix_table_page = NULL;
}
+static void reset_assigned_device(void *opaque)
+{
+PCIDevice *d = (PCIDevice *)opaque;
+uint32_t conf;
+
+/* reset the bus master bit to avoid further DMA transfers */
+conf = assigned_dev_pci_read_config(d, 0x04, 0x02);
+conf &= ~0x04;
+assigned_dev_pci_write_config(d, 0x04, conf, 0x02);
+}
+
static int assigned_initfn(struct PCIDevice *pci_dev)
{
AssignedDevice *dev = DO_UPCAST(AssignedDevice, dev, pci_dev);
@@ -1499,6 +1510,9 @@ static int assigned_initfn(struct PCIDevice *pci_dev)
if (r < 0)
goto assigned_out;
+/* register reset function for the device */
+qemu_register_reset(reset_assigned_device, pci_dev);
+
/* intercept MSI-X entry page in the MMIO */
if (dev->cap.available & ASSIGNED_DEVICE_CAP_MSIX)
if (assigned_dev_register_msix_mmio(dev))
--
1.7.2.2
--
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
