Re: [Rust-VMM] vhost reply_ack negotiation (a.k.a differences in vhost-user behaviour with libvhost-user and vhost-user-backend.rs)
I just found this thread in my email junk box:( I do have found some bugs in the vhost_rs crate, related to handle the need_reply flag. But that bug only affects virtio-fs fs_map operations. Please refer to: https://github.com/rust-vmm/vhost/pull/19 https://github.com/rust-vmm/vhost/pull/19/commits/a2c5a4f50e45ae1ab78622dda9a3f861bd43a17e Thanks, Gerry > On Feb 22, 2021, at 9:27 PM, Dr. David Alan Gilbert > wrote: > > * Alex Bennée (alex.ben...@linaro.org) wrote: >> >> Dr. David Alan Gilbert writes: >> >>> * Alex Bennée (alex.ben...@linaro.org) wrote: Hi, I finally got a chance to get down into the guts of vhost-user while attempting to port my original C RPMB daemon to Rust using the vhost-user-backend and related crates. I ended up with this hang during negotiation: startup vhost_user_write req:1 flags:0x1 vhost_user_read_start vhost_user_read req:1 flags:0x5 vhost_user_backend_init: we got 17000 >> >> GET_FEATURES >> vhost_user_write req:15 flags:0x1 vhost_user_read_start vhost_user_read req:15 flags:0x5 vhost_user_set_protocol_features: 2008 vhost_user_write req:16 flags:0x1 vhost_user_write req:3 flags:0x1 vhost_user_write req:1 flags:0x1 vhost_user_read_start vhost_user_read req:1 flags:0x5 vhost_user_write req:13 flags:0x1 kernel initialises device virtio_rpmb virtio1: init done! vhost_user_write req:13 flags:0x1 vhost_dev_set_features: 13000 vhost_user_set_features: 13000 >> >> SET_FEATURES >> vhost_user_write req:2 flags:0x1 vhost_user_write req:5 flags:0x9 vhost_user_read_start >> - Should QEMU have preserved VhostUserVirtioFeatures::PROTOCOL_FEATURES when doing the eventual VHOST_USER_SET_FEATURES reply? - Is vhost.rs being to strict or libvhost-user too lax in interpreting the negotiated features before processing the ``need_reply`` [Bit 3] field of the messages? >>> >>> I think vhost.rs is being correctly strict - but there would be no harm >>> in it flagging that you'd hit an inconsistency if it finds a need_reply >>> without the feature. >> >> But the feature should have been negotiated. So unless the slave can >> assume it is enabled because it asked I think QEMU is in the wrong by >> not preserving the feature bits in it's SET_FEATURES reply. We just gets >> away with it with libvhostuser being willing to reply anyway. > > Oh I wasn't trying to reply to that bit; I can never remember how the > vhost/virtio feature bit negotiation works... > > Dave > >>> - are VHOST_USER_SET_MEM_TABLE to VHOST_USER_SET_INFLIGHT_FD included in the "list of the ones that do" require replies or do they only reply when REPLY_ACK has been negotiated as the ambiguous "seealso::" box out seems to imply? >>> >>> set_mem_table gives a reply when postcopy is enabled (and then qemu >>> replies to the reply!) but otherwise doesn't. >>> (Note there's an issue opened for .rs to support ADD_MEM_REGION >>> since it's a lot better than SET_MEM_TABLE which has a fixed size table >>> that's small). >> >> Thanks for the heads up. >> >>> >>> Dave >>> Currently I have some hacks in: https://github.com/stsquad/vhost/tree/my-hacks which gets my daemon booting up to the point we actually need to do a transaction. However I won't submit a PR until I've worked out exactly where the problems are. -- Alex Bennée >> >> >> -- >> Alex Bennée >> > -- > Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK > > > ___ > Rust-vmm mailing list > rust-...@lists.opendev.org > http://lists.opendev.org/cgi-bin/mailman/listinfo/rust-vmm
Re: [Qemu-devel] [SeaBIOS] [RESEND PATCH v3] hotplug: add device per func in ACPI DSDT tables
On 05/11/2012 06:14 PM, Gleb Natapov wrote: >> I'm not familiar with qemu:( >> On native OS, admin could trigger PCI device hotplug operations through >> /sys/bus/pci/slot/xx/power. Not sure whether that's needed for guest OS too. >> > Why is it needed on physical HW? May be it is needed in a VM for the > same reason? As Amos has mentioned, it's used power on/off a PCI device instead of physical hotplug. Not sure whether it's needed in guest OS. --gerry
Re: [Qemu-devel] [PATCH v3] pci: clean all funcs when hot-removing multifunc device
On 05/11/2012 08:24 AM, Amos Kong wrote:
> On 05/11/2012 07:54 AM, Amos Kong wrote:
>> On 05/11/2012 02:55 AM, Michael S. Tsirkin wrote:
>>> On Fri, May 11, 2012 at 01:09:13AM +0800, Jiang Liu wrote:
>>>> On 05/10/2012 11:44 PM, Amos Kong wrote:
>>>>
>>>>> diff --git a/drivers/pci/hotplug/acpiphp_glue.c
>>>>> b/drivers/pci/hotplug/acpiphp_glue.c
>>>>> index 806c44f..a7442d9 100644
>>>>> --- a/drivers/pci/hotplug/acpiphp_glue.c
>>>>> +++ b/drivers/pci/hotplug/acpiphp_glue.c
>>>>> @@ -885,7 +885,7 @@ static void disable_bridges(struct pci_bus *bus)
>>>>> static int disable_device(struct acpiphp_slot *slot)
>>>>> {
>>>>> struct acpiphp_func *func;
>>>>> - struct pci_dev *pdev;
>>>>> + struct pci_dev *pdev, *tmp;
>>>>> struct pci_bus *bus = slot->bridge->pci_bus;
>>>>>
>>>>> /* The slot will be enabled when func 0 is added, so check
>>>>> @@ -902,9 +902,10 @@ static int disable_device(struct acpiphp_slot *slot)
>>>>> func->bridge = NULL;
>>>>> }
>>>>>
>>>>> - pdev = pci_get_slot(slot->bridge->pci_bus,
>>>>> - PCI_DEVFN(slot->device, func->function));
>>>>> - if (pdev) {
>>>>> + list_for_each_entry_safe(pdev, tmp, &bus->devices, bus_list) {
>>>>> + if (PCI_SLOT(pdev->devfn) != slot->device)
>>>>> + continue;
>>>>> +
>>>> The pci_bus_sem lock should be acquired when walking the bus->devices list.
>>>> Otherwise it may cause invalid memory access if another thread is modifying
>>>> the bus->devices list concurrently.
>
> pci_bus_sem lock is only request for writing &bus->devices list, right ?
> and this protection already exists in pci_destory_dev().
That's for writer. For reader to walk the pci_bus->devices list, you also need
to acquire the reader lock by down_read(&pci_bus_sem). Please refer to
pci_get_slot() for example. This especially import for native OS because there
may be multiple PCI slots/devices on the bus.
Re: [Qemu-devel] [RESEND PATCH v3] hotplug: add device per func in ACPI DSDT tables
On 05/09/2012 03:24 PM, Amos Kong wrote:
> ---
> src/ssdt-pcihp.dsl | 17
> src/ssdt-pcihp.hex | 8869
> +++-
> 2 files changed, 8781 insertions(+), 105 deletions(-)
>
> diff --git a/src/ssdt-pcihp.dsl b/src/ssdt-pcihp.dsl
> index 4b435b8..2a3c326 100644
> --- a/src/ssdt-pcihp.dsl
> +++ b/src/ssdt-pcihp.dsl
> @@ -17,14 +17,23 @@ DefinitionBlock ("ssdt-pcihp.aml", "SSDT", 0x01, "BXPC",
> "BXSSDTPCIHP", 0x1)
> // at runtime, if the slot is detected to not support hotplug.
> // Extract the offset of the address dword and the
> // _EJ0 name to allow this patching.
> -#define hotplug_slot(slot) \
> -Device (S##slot) { \
> +#define hotplug_func(slot, fn) \
> +Device (S##slot##fn) { \
> ACPI_EXTRACT_NAME_DWORD_CONST aml_adr_dword \
> - Name (_ADR, 0x##slot##) \
> + Name (_ADR, 0x##slot##000##fn) \
> ACPI_EXTRACT_METHOD_STRING aml_ej0_name \
> Method (_EJ0, 1) { Return(PCEJ(0x##slot)) } \
> Name (_SUN, 0x##slot)\
> }
It would be perfect if the Device object could also support _PS0 and _STA
methods. Could we re-add the slot back after hot-removing it from the guest
OS with this ACPI implementation? Say execute following scripts from guest OS.
echo 0 > /sys/bus/pci/slot/xx/power
echo 1 > /sys/bus/pci/slot/xx/power
> +#define hotplug_slot(slot) \
> +hotplug_func(slot, 0) \
> +hotplug_func(slot, 1) \
> +hotplug_func(slot, 2) \
> +hotplug_func(slot, 3) \
> +hotplug_func(slot, 4) \
> +hotplug_func(slot, 5) \
> +hotplug_func(slot, 6) \
> +hotplug_func(slot, 7)
>
> hotplug_slot(01)
> hotplug_slot(02)
> @@ -59,7 +68,7 @@ DefinitionBlock ("ssdt-pcihp.aml", "SSDT", 0x01, "BXPC",
> "BXSSDTPCIHP", 0x1)
> hotplug_slot(1f)
>
> #define gen_pci_hotplug(slot) \
> -If (LEqual(Arg0, 0x##slot)) { Notify(S##slot, Arg1) }
> +If (LEqual(Arg0, 0x##slot)) { Notify(S##slot##0, Arg1) }
>
> Method(PCNT, 2) {
> gen_pci_hotplug(01)
Re: [Qemu-devel] [PATCH v3] pci: clean all funcs when hot-removing multifunc device
On 05/10/2012 11:44 PM, Amos Kong wrote:
> diff --git a/drivers/pci/hotplug/acpiphp_glue.c
> b/drivers/pci/hotplug/acpiphp_glue.c
> index 806c44f..a7442d9 100644
> --- a/drivers/pci/hotplug/acpiphp_glue.c
> +++ b/drivers/pci/hotplug/acpiphp_glue.c
> @@ -885,7 +885,7 @@ static void disable_bridges(struct pci_bus *bus)
> static int disable_device(struct acpiphp_slot *slot)
> {
> struct acpiphp_func *func;
> - struct pci_dev *pdev;
> + struct pci_dev *pdev, *tmp;
> struct pci_bus *bus = slot->bridge->pci_bus;
>
> /* The slot will be enabled when func 0 is added, so check
> @@ -902,9 +902,10 @@ static int disable_device(struct acpiphp_slot *slot)
> func->bridge = NULL;
> }
>
> - pdev = pci_get_slot(slot->bridge->pci_bus,
> - PCI_DEVFN(slot->device, func->function));
> - if (pdev) {
> + list_for_each_entry_safe(pdev, tmp, &bus->devices, bus_list) {
> + if (PCI_SLOT(pdev->devfn) != slot->device)
> + continue;
> +
The pci_bus_sem lock should be acquired when walking the bus->devices list.
Otherwise it may cause invalid memory access if another thread is modifying
the bus->devices list concurrently.
BTW, what's the relationship with "[PATCH v3] hotplug: add device per func
in ACPI DSDT tables"? Seems they are both solving the same issue.
> pci_stop_bus_device(pdev);
> if (pdev->subordinate) {
> disable_bridges(pdev->subordinate);
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" 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] [RESEND PATCH v3] hotplug: add device per func in ACPI DSDT tables
On 05/09/2012 09:47 PM, Alex Williamson wrote:
>> ---
>> src/ssdt-pcihp.dsl | 17
>> src/ssdt-pcihp.hex | 8869
>> +++-
>> 2 files changed, 8781 insertions(+), 105 deletions(-)
>>
>> diff --git a/src/ssdt-pcihp.dsl b/src/ssdt-pcihp.dsl
>> index 4b435b8..2a3c326 100644
>> --- a/src/ssdt-pcihp.dsl
>> +++ b/src/ssdt-pcihp.dsl
>> @@ -17,14 +17,23 @@ DefinitionBlock ("ssdt-pcihp.aml", "SSDT", 0x01, "BXPC",
>> "BXSSDTPCIHP", 0x1)
>> // at runtime, if the slot is detected to not support hotplug.
>> // Extract the offset of the address dword and the
>> // _EJ0 name to allow this patching.
>> -#define hotplug_slot(slot) \
>> -Device (S##slot) { \
>> +#define hotplug_func(slot, fn) \
>> +Device (S##slot##fn) { \
>> ACPI_EXTRACT_NAME_DWORD_CONST aml_adr_dword \
>> - Name (_ADR, 0x##slot##) \
>
> I would have guessed it to be sufficient to change _ADR to
> 0x##slot##, does that not work? Thanks,
>
> Alex
Currently Linux acpiphp driver doesn't support that yet.
--gerry
Re: [Qemu-devel] [RESEND PATCH v3] hotplug: add device per func in ACPI DSDT tables
On 05/11/2012 01:42 AM, Michael S. Tsirkin wrote:
> On Fri, May 11, 2012 at 01:17:38AM +0800, Jiang Liu wrote:
>> On 05/09/2012 03:24 PM, Amos Kong wrote:
>>
>>> ---
>>> src/ssdt-pcihp.dsl | 17
>>> src/ssdt-pcihp.hex | 8869
>>> +++-
>>> 2 files changed, 8781 insertions(+), 105 deletions(-)
>>>
>>> diff --git a/src/ssdt-pcihp.dsl b/src/ssdt-pcihp.dsl
>>> index 4b435b8..2a3c326 100644
>>> --- a/src/ssdt-pcihp.dsl
>>> +++ b/src/ssdt-pcihp.dsl
>>> @@ -17,14 +17,23 @@ DefinitionBlock ("ssdt-pcihp.aml", "SSDT", 0x01,
>>> "BXPC", "BXSSDTPCIHP", 0x1)
>>> // at runtime, if the slot is detected to not support hotplug.
>>> // Extract the offset of the address dword and the
>>> // _EJ0 name to allow this patching.
>>> -#define hotplug_slot(slot) \
>>> -Device (S##slot) { \
>>> +#define hotplug_func(slot, fn) \
>>> +Device (S##slot##fn) { \
>>> ACPI_EXTRACT_NAME_DWORD_CONST aml_adr_dword \
>>> - Name (_ADR, 0x##slot##) \
>>> + Name (_ADR, 0x##slot##000##fn) \
>>> ACPI_EXTRACT_METHOD_STRING aml_ej0_name \
>>> Method (_EJ0, 1) { Return(PCEJ(0x##slot)) } \
>>> Name (_SUN, 0x##slot)\
>>> }
>> It would be perfect if the Device object could also support _PS0 and _STA
>> methods.
>
> It needs qemu support, and some backward compatibility hack.
> Why?
>
>> Could we re-add the slot back after hot-removing it from the guest
>> OS with this ACPI implementation? Say execute following scripts from guest
>> OS.
>> echo 0 > /sys/bus/pci/slot/xx/power
>> echo 1 > /sys/bus/pci/slot/xx/power
>
> No because qemu removes device after eject.
> Do you have a need for this functionality? What is it?
I'm not familiar with qemu:(
On native OS, admin could trigger PCI device hotplug operations through
/sys/bus/pci/slot/xx/power. Not sure whether that's needed for guest OS too.
