From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
Add relevant vpci register handlers when assigning PCI device to a domain
and remove those when de-assigning. This allows having different
handlers for different domains, e.g. hwdom and other guests.
Use stubs for guest domains for now.
Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
---
xen/drivers/vpci/header.c | 78 +++++++++++++++++++++++++++++++++++----
xen/drivers/vpci/vpci.c | 4 +-
xen/include/xen/vpci.h | 4 ++
3 files changed, 76 insertions(+), 10 deletions(-)
diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
index 31bca7a12942..5218b1af247e 100644
--- a/xen/drivers/vpci/header.c
+++ b/xen/drivers/vpci/header.c
@@ -397,6 +397,17 @@ static void bar_write(const struct pci_dev *pdev, unsigned int reg,
pci_conf_write32(pdev->sbdf, reg, val);
}
+static void guest_bar_write(const struct pci_dev *pdev, unsigned int reg,
+ uint32_t val, void *data)
+{
+}
+
+static uint32_t guest_bar_read(const struct pci_dev *pdev, unsigned int reg,
+ void *data)
+{
+ return 0xffffffff;
+}
+
static void rom_write(const struct pci_dev *pdev, unsigned int reg,
uint32_t val, void *data)
{
@@ -445,14 +456,25 @@ static void rom_write(const struct pci_dev *pdev, unsigned int reg,
rom->addr = val & PCI_ROM_ADDRESS_MASK;
}
-static int add_bar_handlers(struct pci_dev *pdev)
+static void guest_rom_write(const struct pci_dev *pdev, unsigned int reg,
+ uint32_t val, void *data)
+{
+}
+
+static uint32_t guest_rom_read(const struct pci_dev *pdev, unsigned int reg,
+ void *data)
+{
+ return 0xffffffff;
+}
+
+static int add_bar_handlers(struct pci_dev *pdev, bool is_hwdom)
{
unsigned int i;
struct vpci_header *header = &pdev->vpci->header;
struct vpci_bar *bars = header->bars;
int rc;
- /* Setup a handler for the command register. */
+ /* Setup a handler for the command register: same for hwdom and guests. */
rc = vpci_add_register(pdev->vpci, vpci_hw_read16, cmd_write, PCI_COMMAND,
2, header);
if ( rc )
@@ -475,8 +497,13 @@ static int add_bar_handlers(struct pci_dev *pdev)
rom_reg = PCI_ROM_ADDRESS;
else
rom_reg = PCI_ROM_ADDRESS1;
- rc = vpci_add_register(pdev->vpci, vpci_hw_read32, rom_write,
- rom_reg, 4, &bars[i]);
+ if ( is_hwdom )
+ rc = vpci_add_register(pdev->vpci, vpci_hw_read32, rom_write,
+ rom_reg, 4, &bars[i]);
+ else
+ rc = vpci_add_register(pdev->vpci,
+ guest_rom_read, guest_rom_write,
+ rom_reg, 4, &bars[i]);
if ( rc )
return rc;
}
@@ -485,8 +512,13 @@ static int add_bar_handlers(struct pci_dev *pdev)
uint8_t reg = PCI_BASE_ADDRESS_0 + i * 4;
/* This is either VPCI_BAR_MEM32 or VPCI_BAR_MEM64_{LO|HI}. */
- rc = vpci_add_register(pdev->vpci, vpci_hw_read32, bar_write, reg,
- 4, &bars[i]);
+ if ( is_hwdom )
+ rc = vpci_add_register(pdev->vpci, vpci_hw_read32, bar_write,
+ reg, 4, &bars[i]);
+ else
+ rc = vpci_add_register(pdev->vpci,
+ guest_bar_read, guest_bar_write,
+ reg, 4, &bars[i]);
if ( rc )
return rc;
}
@@ -520,7 +552,7 @@ static int init_bars(struct pci_dev *pdev)
}
if ( pdev->ignore_bars )
- return add_bar_handlers(pdev);
+ return add_bar_handlers(pdev, true);
/* Disable memory decoding before sizing. */
cmd = pci_conf_read16(pdev->sbdf, PCI_COMMAND);
@@ -582,7 +614,7 @@ static int init_bars(struct pci_dev *pdev)
PCI_ROM_ADDRESS_ENABLE;
}
- rc = add_bar_handlers(pdev);
+ rc = add_bar_handlers(pdev, true);
if ( rc )
{
pci_conf_write16(pdev->sbdf, PCI_COMMAND, cmd);
@@ -593,6 +625,36 @@ static int init_bars(struct pci_dev *pdev)
}
REGISTER_VPCI_INIT(init_bars, VPCI_PRIORITY_MIDDLE);
+int vpci_bar_add_handlers(const struct domain *d, struct pci_dev *pdev)
+{
+ int rc;
+
+ /* Remove previously added registers. */
+ vpci_remove_device_registers(pdev);
+
+ /* It only makes sense to add registers for hwdom or guest domain. */
+ if ( d->domain_id >= DOMID_FIRST_RESERVED )
+ return 0;
+
+ if ( is_hardware_domain(d) )
+ rc = add_bar_handlers(pdev, true);
+ else
+ rc = add_bar_handlers(pdev, false);
+
+ if ( rc )
+ gprintk(XENLOG_ERR,
+ "%pp: failed to add BAR handlers for dom%d\n", &pdev->sbdf,
+ d->domain_id);
+ return rc;
+}
+
+int vpci_bar_remove_handlers(const struct domain *d, struct pci_dev *pdev)
+{
+ /* Remove previously added registers. */
+ vpci_remove_device_registers(pdev);
+ return 0;
+}
+
/*
* Local variables:
* mode: C
diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
index ee0ad63a3c12..4530313f01e7 100644
--- a/xen/drivers/vpci/vpci.c
+++ b/xen/drivers/vpci/vpci.c
@@ -94,7 +94,7 @@ int vpci_assign_device(struct domain *d, struct pci_dev *dev)
if ( !has_vpci(d) || (d->domain_id >= DOMID_FIRST_RESERVED) )
return 0;
- return 0;
+ return vpci_bar_add_handlers(d, dev);
}
/* Notify vPCI that device is de-assigned from guest. */
@@ -104,7 +104,7 @@ int vpci_deassign_device(struct domain *d, struct pci_dev *dev)
if ( !has_vpci(d) || (d->domain_id >= DOMID_FIRST_RESERVED) )
return 0;
- return 0;
+ return vpci_bar_remove_handlers(d, dev);
}
#endif /* __XEN__ */
diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h
index e7a1a09ab4c9..4aa2941a1081 100644
--- a/xen/include/xen/vpci.h
+++ b/xen/include/xen/vpci.h
@@ -63,6 +63,10 @@ uint32_t vpci_hw_read32(const struct pci_dev *pdev, unsigned int reg,
*/
bool __must_check vpci_process_pending(struct vcpu *v);
+/* Add/remove BAR handlers for a domain. */
+int vpci_bar_add_handlers(const struct domain *d, struct pci_dev *pdev);
+int vpci_bar_remove_handlers(const struct domain *d, struct pci_dev *pdev);
+
struct vpci {
/* List of vPCI handlers for a device. */
struct list_head handlers;
--
2.25.1
On 03.09.2021 12:08, Oleksandr Andrushchenko wrote:
> @@ -593,6 +625,36 @@ static int init_bars(struct pci_dev *pdev)
> }
> REGISTER_VPCI_INIT(init_bars, VPCI_PRIORITY_MIDDLE);
>
> +int vpci_bar_add_handlers(const struct domain *d, struct pci_dev *pdev)
> +{
> + int rc;
> +
> + /* Remove previously added registers. */
> + vpci_remove_device_registers(pdev);
> +
> + /* It only makes sense to add registers for hwdom or guest domain. */
> + if ( d->domain_id >= DOMID_FIRST_RESERVED )
> + return 0;
> +
> + if ( is_hardware_domain(d) )
> + rc = add_bar_handlers(pdev, true);
> + else
> + rc = add_bar_handlers(pdev, false);
rc = add_bar_handlers(pdev, is_hardware_domain(d));
> + if ( rc )
> + gprintk(XENLOG_ERR,
> + "%pp: failed to add BAR handlers for dom%d\n", &pdev->sbdf,
> + d->domain_id);
Please use %pd and correct indentation. Logging the error code might
also help some in diagnosing issues. Further I'm not sure this is a
message we want in release builds - perhaps gdprintk()?
> + return rc;
> +}
> +
> +int vpci_bar_remove_handlers(const struct domain *d, struct pci_dev *pdev)
> +{
> + /* Remove previously added registers. */
> + vpci_remove_device_registers(pdev);
> + return 0;
> +}
Also - in how far is the goal of your work to also make vPCI work for
x86 DomU-s? If that's not a goal, I'd like to ask that you limit the
introduction of code that ends up dead there.
Jan
On 06.09.21 17:11, Jan Beulich wrote:
> On 03.09.2021 12:08, Oleksandr Andrushchenko wrote:
>> @@ -593,6 +625,36 @@ static int init_bars(struct pci_dev *pdev)
>> }
>> REGISTER_VPCI_INIT(init_bars, VPCI_PRIORITY_MIDDLE);
>>
>> +int vpci_bar_add_handlers(const struct domain *d, struct pci_dev *pdev)
>> +{
>> + int rc;
>> +
>> + /* Remove previously added registers. */
>> + vpci_remove_device_registers(pdev);
>> +
>> + /* It only makes sense to add registers for hwdom or guest domain. */
>> + if ( d->domain_id >= DOMID_FIRST_RESERVED )
>> + return 0;
>> +
>> + if ( is_hardware_domain(d) )
>> + rc = add_bar_handlers(pdev, true);
>> + else
>> + rc = add_bar_handlers(pdev, false);
> rc = add_bar_handlers(pdev, is_hardware_domain(d));
Indeed, thank you ;)
>
>> + if ( rc )
>> + gprintk(XENLOG_ERR,
>> + "%pp: failed to add BAR handlers for dom%d\n", &pdev->sbdf,
>> + d->domain_id);
> Please use %pd and correct indentation. Logging the error code might
> also help some in diagnosing issues.
Sure, I'll change it to %pd
> Further I'm not sure this is a
> message we want in release builds
Why not?
> - perhaps gdprintk()?
I'll change if we decide so
>
>> + return rc;
>> +}
>> +
>> +int vpci_bar_remove_handlers(const struct domain *d, struct pci_dev *pdev)
>> +{
>> + /* Remove previously added registers. */
>> + vpci_remove_device_registers(pdev);
>> + return 0;
>> +}
> Also - in how far is the goal of your work to also make vPCI work for
> x86 DomU-s? If that's not a goal
It is not, unfortunately. The goal is not to break x86 and to enable Arm
> , I'd like to ask that you limit the
> introduction of code that ends up dead there.
What's wrong with this function even if it is a one-liner?
This way we have a pair vpci_bar_add_handlers/vpci_bar_remove_handlers
and if I understood correctly you suggest vpci_bar_add_handlers/vpci_remove_device_registers?
What would we gain from that, but yet another secret knowledge that in order
to remove BAR handlers one needs to call vpci_remove_device_registers
while I would personally expect to call vpci_bar_add_handlers' counterpart,
vpci_remove_device_registers namely.
> Jan
>
Thank you,
Oleksandr
On 07.09.2021 12:11, Oleksandr Andrushchenko wrote:
> On 06.09.21 17:11, Jan Beulich wrote:
>> On 03.09.2021 12:08, Oleksandr Andrushchenko wrote:
>>> @@ -593,6 +625,36 @@ static int init_bars(struct pci_dev *pdev)
>>> }
>>> REGISTER_VPCI_INIT(init_bars, VPCI_PRIORITY_MIDDLE);
>>>
>>> +int vpci_bar_add_handlers(const struct domain *d, struct pci_dev *pdev)
>>> +{
>>> + int rc;
>>> +
>>> + /* Remove previously added registers. */
>>> + vpci_remove_device_registers(pdev);
>>> +
>>> + /* It only makes sense to add registers for hwdom or guest domain. */
>>> + if ( d->domain_id >= DOMID_FIRST_RESERVED )
>>> + return 0;
>>> +
>>> + if ( is_hardware_domain(d) )
>>> + rc = add_bar_handlers(pdev, true);
>>> + else
>>> + rc = add_bar_handlers(pdev, false);
>> rc = add_bar_handlers(pdev, is_hardware_domain(d));
> Indeed, thank you ;)
>>
>>> + if ( rc )
>>> + gprintk(XENLOG_ERR,
>>> + "%pp: failed to add BAR handlers for dom%d\n", &pdev->sbdf,
>>> + d->domain_id);
>> Please use %pd and correct indentation. Logging the error code might
>> also help some in diagnosing issues.
> Sure, I'll change it to %pd
>> Further I'm not sure this is a
>> message we want in release builds
> Why not?
Excess verbosity: If we have such here, why not elsewhere on error paths?
And I hope you agree things will get too verbose if we had such (about)
everywhere?
>> - perhaps gdprintk()?
> I'll change if we decide so
>>
>>> + return rc;
>>> +}
>>> +
>>> +int vpci_bar_remove_handlers(const struct domain *d, struct pci_dev *pdev)
>>> +{
>>> + /* Remove previously added registers. */
>>> + vpci_remove_device_registers(pdev);
>>> + return 0;
>>> +}
>> Also - in how far is the goal of your work to also make vPCI work for
>> x86 DomU-s? If that's not a goal
> It is not, unfortunately. The goal is not to break x86 and to enable Arm
>> , I'd like to ask that you limit the
>> introduction of code that ends up dead there.
>
> What's wrong with this function even if it is a one-liner?
The comment is primarily on the earlier function, and then extends to
this one.
> This way we have a pair vpci_bar_add_handlers/vpci_bar_remove_handlers
> and if I understood correctly you suggest vpci_bar_add_handlers/vpci_remove_device_registers?
> What would we gain from that, but yet another secret knowledge that in order
> to remove BAR handlers one needs to call vpci_remove_device_registers
> while I would personally expect to call vpci_bar_add_handlers' counterpart,
> vpci_remove_device_registers namely.
This is all fine. Yet vpci_bar_{add,remove}_handlers() will, aiui, be
dead code on x86. Hence there should be an arrangement allowing the
compiler to eliminate this dead code. Whether that's enclosing these
by "#ifdef" or adding early "if(!IS_ENABLED(CONFIG_*))" is secondary.
This has a knock-on effect on other functions as you certainly realize:
The compiler seeing e.g. the 2nd argument to the add-BARs function
always being true allows it to instantiate just a clone of the original
function with the respective conditionals removed.
Jan
On 07.09.21 13:43, Jan Beulich wrote:
> On 07.09.2021 12:11, Oleksandr Andrushchenko wrote:
>> On 06.09.21 17:11, Jan Beulich wrote:
>>> On 03.09.2021 12:08, Oleksandr Andrushchenko wrote:
>>>> @@ -593,6 +625,36 @@ static int init_bars(struct pci_dev *pdev)
>>>> }
>>>> REGISTER_VPCI_INIT(init_bars, VPCI_PRIORITY_MIDDLE);
>>>>
>>>> +int vpci_bar_add_handlers(const struct domain *d, struct pci_dev *pdev)
>>>> +{
>>>> + int rc;
>>>> +
>>>> + /* Remove previously added registers. */
>>>> + vpci_remove_device_registers(pdev);
>>>> +
>>>> + /* It only makes sense to add registers for hwdom or guest domain. */
>>>> + if ( d->domain_id >= DOMID_FIRST_RESERVED )
>>>> + return 0;
>>>> +
>>>> + if ( is_hardware_domain(d) )
>>>> + rc = add_bar_handlers(pdev, true);
>>>> + else
>>>> + rc = add_bar_handlers(pdev, false);
>>> rc = add_bar_handlers(pdev, is_hardware_domain(d));
>> Indeed, thank you ;)
>>>> + if ( rc )
>>>> + gprintk(XENLOG_ERR,
>>>> + "%pp: failed to add BAR handlers for dom%d\n", &pdev->sbdf,
>>>> + d->domain_id);
>>> Please use %pd and correct indentation. Logging the error code might
>>> also help some in diagnosing issues.
>> Sure, I'll change it to %pd
>>> Further I'm not sure this is a
>>> message we want in release builds
>> Why not?
> Excess verbosity: If we have such here, why not elsewhere on error paths?
> And I hope you agree things will get too verbose if we had such (about)
> everywhere?
Agree, will change it to gdprintk
>
>>> - perhaps gdprintk()?
>> I'll change if we decide so
>>>> + return rc;
>>>> +}
>>>> +
>>>> +int vpci_bar_remove_handlers(const struct domain *d, struct pci_dev *pdev)
>>>> +{
>>>> + /* Remove previously added registers. */
>>>> + vpci_remove_device_registers(pdev);
>>>> + return 0;
>>>> +}
>>> Also - in how far is the goal of your work to also make vPCI work for
>>> x86 DomU-s? If that's not a goal
>> It is not, unfortunately. The goal is not to break x86 and to enable Arm
>>> , I'd like to ask that you limit the
>>> introduction of code that ends up dead there.
>> What's wrong with this function even if it is a one-liner?
> The comment is primarily on the earlier function, and then extends to
> this one.
>
>> This way we have a pair vpci_bar_add_handlers/vpci_bar_remove_handlers
>> and if I understood correctly you suggest vpci_bar_add_handlers/vpci_remove_device_registers?
>> What would we gain from that, but yet another secret knowledge that in order
>> to remove BAR handlers one needs to call vpci_remove_device_registers
>> while I would personally expect to call vpci_bar_add_handlers' counterpart,
>> vpci_remove_device_registers namely.
> This is all fine. Yet vpci_bar_{add,remove}_handlers() will, aiui, be
> dead code on x86.
vpci_bar_add_handlers will be used by x86 PVH Dom0
> Hence there should be an arrangement allowing the
> compiler to eliminate this dead code.
So, the only dead code for x86 here will be vpci_bar_remove_handlers. Yet.
Because I hope x86 to gain guest support for PVH Dom0 sooner or later.
> Whether that's enclosing these
> by "#ifdef" or adding early "if(!IS_ENABLED(CONFIG_*))" is secondary.
> This has a knock-on effect on other functions as you certainly realize:
> The compiler seeing e.g. the 2nd argument to the add-BARs function
> always being true allows it to instantiate just a clone of the original
> function with the respective conditionals removed.
With the above (e.g. add is going to be used, but not remove) do you
think it is worth playing with ifdef's to strip that single function and add
a piece of spaghetti code to save a bit? What would that ifdef look like,
e.g. #ifdef CONFIG_ARM or #ifndef CONFIG_X86 && any other platform, but ARM?
IMO, it is cleaner to leave it as is. Yet we waste some bits for x86.
>
> Jan
>
Thank you,
Oleksandr
On 07.09.2021 13:10, Oleksandr Andrushchenko wrote:
>
> On 07.09.21 13:43, Jan Beulich wrote:
>> On 07.09.2021 12:11, Oleksandr Andrushchenko wrote:
>>> On 06.09.21 17:11, Jan Beulich wrote:
>>>> On 03.09.2021 12:08, Oleksandr Andrushchenko wrote:
>>>>> @@ -593,6 +625,36 @@ static int init_bars(struct pci_dev *pdev)
>>>>> }
>>>>> REGISTER_VPCI_INIT(init_bars, VPCI_PRIORITY_MIDDLE);
>>>>>
>>>>> +int vpci_bar_add_handlers(const struct domain *d, struct pci_dev *pdev)
>>>>> +{
>>>>> + int rc;
>>>>> +
>>>>> + /* Remove previously added registers. */
>>>>> + vpci_remove_device_registers(pdev);
>>>>> +
>>>>> + /* It only makes sense to add registers for hwdom or guest domain. */
>>>>> + if ( d->domain_id >= DOMID_FIRST_RESERVED )
>>>>> + return 0;
>>>>> +
>>>>> + if ( is_hardware_domain(d) )
>>>>> + rc = add_bar_handlers(pdev, true);
>>>>> + else
>>>>> + rc = add_bar_handlers(pdev, false);
>>>> rc = add_bar_handlers(pdev, is_hardware_domain(d));
>>> Indeed, thank you ;)
>>>>> + if ( rc )
>>>>> + gprintk(XENLOG_ERR,
>>>>> + "%pp: failed to add BAR handlers for dom%d\n", &pdev->sbdf,
>>>>> + d->domain_id);
>>>> Please use %pd and correct indentation. Logging the error code might
>>>> also help some in diagnosing issues.
>>> Sure, I'll change it to %pd
>>>> Further I'm not sure this is a
>>>> message we want in release builds
>>> Why not?
>> Excess verbosity: If we have such here, why not elsewhere on error paths?
>> And I hope you agree things will get too verbose if we had such (about)
>> everywhere?
> Agree, will change it to gdprintk
>>
>>>> - perhaps gdprintk()?
>>> I'll change if we decide so
>>>>> + return rc;
>>>>> +}
>>>>> +
>>>>> +int vpci_bar_remove_handlers(const struct domain *d, struct pci_dev *pdev)
>>>>> +{
>>>>> + /* Remove previously added registers. */
>>>>> + vpci_remove_device_registers(pdev);
>>>>> + return 0;
>>>>> +}
>>>> Also - in how far is the goal of your work to also make vPCI work for
>>>> x86 DomU-s? If that's not a goal
>>> It is not, unfortunately. The goal is not to break x86 and to enable Arm
>>>> , I'd like to ask that you limit the
>>>> introduction of code that ends up dead there.
>>> What's wrong with this function even if it is a one-liner?
>> The comment is primarily on the earlier function, and then extends to
>> this one.
>>
>>> This way we have a pair vpci_bar_add_handlers/vpci_bar_remove_handlers
>>> and if I understood correctly you suggest vpci_bar_add_handlers/vpci_remove_device_registers?
>>> What would we gain from that, but yet another secret knowledge that in order
>>> to remove BAR handlers one needs to call vpci_remove_device_registers
>>> while I would personally expect to call vpci_bar_add_handlers' counterpart,
>>> vpci_remove_device_registers namely.
>> This is all fine. Yet vpci_bar_{add,remove}_handlers() will, aiui, be
>> dead code on x86.
> vpci_bar_add_handlers will be used by x86 PVH Dom0
Where / when? You add a call from vpci_assign_device(), but besides that
also being dead code on x86 (for now), you can't mean that because
vpci_deassign_device() also calls vpci_bar_remove_handlers().
>> Hence there should be an arrangement allowing the
>> compiler to eliminate this dead code.
>
> So, the only dead code for x86 here will be vpci_bar_remove_handlers. Yet.
> Because I hope x86 to gain guest support for PVH Dom0 sooner or later.
>
>> Whether that's enclosing these
>> by "#ifdef" or adding early "if(!IS_ENABLED(CONFIG_*))" is secondary.
>> This has a knock-on effect on other functions as you certainly realize:
>> The compiler seeing e.g. the 2nd argument to the add-BARs function
>> always being true allows it to instantiate just a clone of the original
>> function with the respective conditionals removed.
>
> With the above (e.g. add is going to be used, but not remove) do you
> think it is worth playing with ifdef's to strip that single function and add
> a piece of spaghetti code to save a bit?
No, that I agree wouldn't be worth it.
> What would that ifdef look like,
> e.g. #ifdef CONFIG_ARM or #ifndef CONFIG_X86 && any other platform, but ARM?
A new setting, preferably; e.g. VCPU_UNPRIVILEGED, to be "select"ed by
architectures as functionality gets enabled.
Jan
On 07.09.21 14:49, Jan Beulich wrote:
> On 07.09.2021 13:10, Oleksandr Andrushchenko wrote:
>> On 07.09.21 13:43, Jan Beulich wrote:
>>> On 07.09.2021 12:11, Oleksandr Andrushchenko wrote:
>>>> On 06.09.21 17:11, Jan Beulich wrote:
>>>>> On 03.09.2021 12:08, Oleksandr Andrushchenko wrote:
>>>>>> @@ -593,6 +625,36 @@ static int init_bars(struct pci_dev *pdev)
>>>>>> }
>>>>>> REGISTER_VPCI_INIT(init_bars, VPCI_PRIORITY_MIDDLE);
>>>>>>
>>>>>> +int vpci_bar_add_handlers(const struct domain *d, struct pci_dev *pdev)
>>>>>> +{
>>>>>> + int rc;
>>>>>> +
>>>>>> + /* Remove previously added registers. */
>>>>>> + vpci_remove_device_registers(pdev);
>>>>>> +
>>>>>> + /* It only makes sense to add registers for hwdom or guest domain. */
>>>>>> + if ( d->domain_id >= DOMID_FIRST_RESERVED )
>>>>>> + return 0;
>>>>>> +
>>>>>> + if ( is_hardware_domain(d) )
>>>>>> + rc = add_bar_handlers(pdev, true);
>>>>>> + else
>>>>>> + rc = add_bar_handlers(pdev, false);
>>>>> rc = add_bar_handlers(pdev, is_hardware_domain(d));
>>>> Indeed, thank you ;)
>>>>>> + if ( rc )
>>>>>> + gprintk(XENLOG_ERR,
>>>>>> + "%pp: failed to add BAR handlers for dom%d\n", &pdev->sbdf,
>>>>>> + d->domain_id);
>>>>> Please use %pd and correct indentation. Logging the error code might
>>>>> also help some in diagnosing issues.
>>>> Sure, I'll change it to %pd
>>>>> Further I'm not sure this is a
>>>>> message we want in release builds
>>>> Why not?
>>> Excess verbosity: If we have such here, why not elsewhere on error paths?
>>> And I hope you agree things will get too verbose if we had such (about)
>>> everywhere?
>> Agree, will change it to gdprintk
>>>>> - perhaps gdprintk()?
>>>> I'll change if we decide so
>>>>>> + return rc;
>>>>>> +}
>>>>>> +
>>>>>> +int vpci_bar_remove_handlers(const struct domain *d, struct pci_dev *pdev)
>>>>>> +{
>>>>>> + /* Remove previously added registers. */
>>>>>> + vpci_remove_device_registers(pdev);
>>>>>> + return 0;
>>>>>> +}
>>>>> Also - in how far is the goal of your work to also make vPCI work for
>>>>> x86 DomU-s? If that's not a goal
>>>> It is not, unfortunately. The goal is not to break x86 and to enable Arm
>>>>> , I'd like to ask that you limit the
>>>>> introduction of code that ends up dead there.
>>>> What's wrong with this function even if it is a one-liner?
>>> The comment is primarily on the earlier function, and then extends to
>>> this one.
>>>
>>>> This way we have a pair vpci_bar_add_handlers/vpci_bar_remove_handlers
>>>> and if I understood correctly you suggest vpci_bar_add_handlers/vpci_remove_device_registers?
>>>> What would we gain from that, but yet another secret knowledge that in order
>>>> to remove BAR handlers one needs to call vpci_remove_device_registers
>>>> while I would personally expect to call vpci_bar_add_handlers' counterpart,
>>>> vpci_remove_device_registers namely.
>>> This is all fine. Yet vpci_bar_{add,remove}_handlers() will, aiui, be
>>> dead code on x86.
>> vpci_bar_add_handlers will be used by x86 PVH Dom0
> Where / when? You add a call from vpci_assign_device(), but besides that
> also being dead code on x86 (for now), you can't mean that because
> vpci_deassign_device() also calls vpci_bar_remove_handlers().
You are right here and both add/remove are not used on x86 PVH Dom0.
I am sorry for wasting your time
>
>>> Hence there should be an arrangement allowing the
>>> compiler to eliminate this dead code.
>> So, the only dead code for x86 here will be vpci_bar_remove_handlers. Yet.
>> Because I hope x86 to gain guest support for PVH Dom0 sooner or later.
>>
>>> Whether that's enclosing these
>>> by "#ifdef" or adding early "if(!IS_ENABLED(CONFIG_*))" is secondary.
>>> This has a knock-on effect on other functions as you certainly realize:
>>> The compiler seeing e.g. the 2nd argument to the add-BARs function
>>> always being true allows it to instantiate just a clone of the original
>>> function with the respective conditionals removed.
>> With the above (e.g. add is going to be used, but not remove) do you
>> think it is worth playing with ifdef's to strip that single function and add
>> a piece of spaghetti code to save a bit?
> No, that I agree wouldn't be worth it.
>
>> What would that ifdef look like,
>> e.g. #ifdef CONFIG_ARM or #ifndef CONFIG_X86 && any other platform, but ARM?
> A new setting, preferably; e.g. VCPU_UNPRIVILEGED, to be "select"ed by
> architectures as functionality gets enabled.
So, as add/remove are only needed for Arm at the moment
you suggest I add VCPU_UNPRIVILEGED to Arm's Kconfig to enable
compiling vpci_bar_add_handlers/vpci_bar_remove_handlers?
To me this is more about vPCI's support for guests, so should we probably call it
VPCI_XXX instead? E.g. VPCI_HAS_GUEST_SUPPORT or something which
will reflect the nature of the code being gated? VCPU_UNPRIVILEGED sounds
like not connected to vpci to me
>
> Jan
>
Thank you,
Oleksandr
On 07.09.2021 14:16, Oleksandr Andrushchenko wrote:
>
> On 07.09.21 14:49, Jan Beulich wrote:
>> On 07.09.2021 13:10, Oleksandr Andrushchenko wrote:
>>> On 07.09.21 13:43, Jan Beulich wrote:
>>>> On 07.09.2021 12:11, Oleksandr Andrushchenko wrote:
>>>>> On 06.09.21 17:11, Jan Beulich wrote:
>>>>>> On 03.09.2021 12:08, Oleksandr Andrushchenko wrote:
>>>>>>> @@ -593,6 +625,36 @@ static int init_bars(struct pci_dev *pdev)
>>>>>>> }
>>>>>>> REGISTER_VPCI_INIT(init_bars, VPCI_PRIORITY_MIDDLE);
>>>>>>>
>>>>>>> +int vpci_bar_add_handlers(const struct domain *d, struct pci_dev *pdev)
>>>>>>> +{
>>>>>>> + int rc;
>>>>>>> +
>>>>>>> + /* Remove previously added registers. */
>>>>>>> + vpci_remove_device_registers(pdev);
>>>>>>> +
>>>>>>> + /* It only makes sense to add registers for hwdom or guest domain. */
>>>>>>> + if ( d->domain_id >= DOMID_FIRST_RESERVED )
>>>>>>> + return 0;
>>>>>>> +
>>>>>>> + if ( is_hardware_domain(d) )
>>>>>>> + rc = add_bar_handlers(pdev, true);
>>>>>>> + else
>>>>>>> + rc = add_bar_handlers(pdev, false);
>>>>>> rc = add_bar_handlers(pdev, is_hardware_domain(d));
>>>>> Indeed, thank you ;)
>>>>>>> + if ( rc )
>>>>>>> + gprintk(XENLOG_ERR,
>>>>>>> + "%pp: failed to add BAR handlers for dom%d\n", &pdev->sbdf,
>>>>>>> + d->domain_id);
>>>>>> Please use %pd and correct indentation. Logging the error code might
>>>>>> also help some in diagnosing issues.
>>>>> Sure, I'll change it to %pd
>>>>>> Further I'm not sure this is a
>>>>>> message we want in release builds
>>>>> Why not?
>>>> Excess verbosity: If we have such here, why not elsewhere on error paths?
>>>> And I hope you agree things will get too verbose if we had such (about)
>>>> everywhere?
>>> Agree, will change it to gdprintk
>>>>>> - perhaps gdprintk()?
>>>>> I'll change if we decide so
>>>>>>> + return rc;
>>>>>>> +}
>>>>>>> +
>>>>>>> +int vpci_bar_remove_handlers(const struct domain *d, struct pci_dev *pdev)
>>>>>>> +{
>>>>>>> + /* Remove previously added registers. */
>>>>>>> + vpci_remove_device_registers(pdev);
>>>>>>> + return 0;
>>>>>>> +}
>>>>>> Also - in how far is the goal of your work to also make vPCI work for
>>>>>> x86 DomU-s? If that's not a goal
>>>>> It is not, unfortunately. The goal is not to break x86 and to enable Arm
>>>>>> , I'd like to ask that you limit the
>>>>>> introduction of code that ends up dead there.
>>>>> What's wrong with this function even if it is a one-liner?
>>>> The comment is primarily on the earlier function, and then extends to
>>>> this one.
>>>>
>>>>> This way we have a pair vpci_bar_add_handlers/vpci_bar_remove_handlers
>>>>> and if I understood correctly you suggest vpci_bar_add_handlers/vpci_remove_device_registers?
>>>>> What would we gain from that, but yet another secret knowledge that in order
>>>>> to remove BAR handlers one needs to call vpci_remove_device_registers
>>>>> while I would personally expect to call vpci_bar_add_handlers' counterpart,
>>>>> vpci_remove_device_registers namely.
>>>> This is all fine. Yet vpci_bar_{add,remove}_handlers() will, aiui, be
>>>> dead code on x86.
>>> vpci_bar_add_handlers will be used by x86 PVH Dom0
>> Where / when? You add a call from vpci_assign_device(), but besides that
>> also being dead code on x86 (for now), you can't mean that because
>> vpci_deassign_device() also calls vpci_bar_remove_handlers().
>
> You are right here and both add/remove are not used on x86 PVH Dom0.
>
> I am sorry for wasting your time
>
>>
>>>> Hence there should be an arrangement allowing the
>>>> compiler to eliminate this dead code.
>>> So, the only dead code for x86 here will be vpci_bar_remove_handlers. Yet.
>>> Because I hope x86 to gain guest support for PVH Dom0 sooner or later.
>>>
>>>> Whether that's enclosing these
>>>> by "#ifdef" or adding early "if(!IS_ENABLED(CONFIG_*))" is secondary.
>>>> This has a knock-on effect on other functions as you certainly realize:
>>>> The compiler seeing e.g. the 2nd argument to the add-BARs function
>>>> always being true allows it to instantiate just a clone of the original
>>>> function with the respective conditionals removed.
>>> With the above (e.g. add is going to be used, but not remove) do you
>>> think it is worth playing with ifdef's to strip that single function and add
>>> a piece of spaghetti code to save a bit?
>> No, that I agree wouldn't be worth it.
>>
>>> What would that ifdef look like,
>>> e.g. #ifdef CONFIG_ARM or #ifndef CONFIG_X86 && any other platform, but ARM?
>> A new setting, preferably; e.g. VCPU_UNPRIVILEGED, to be "select"ed by
>> architectures as functionality gets enabled.
>
> So, as add/remove are only needed for Arm at the moment
> you suggest I add VCPU_UNPRIVILEGED to Arm's Kconfig to enable
> compiling vpci_bar_add_handlers/vpci_bar_remove_handlers?
> To me this is more about vPCI's support for guests, so should we probably call it
> VPCI_XXX instead? E.g. VPCI_HAS_GUEST_SUPPORT or something which
> will reflect the nature of the code being gated? VCPU_UNPRIVILEGED sounds
> like not connected to vpci to me
And validly so - my fingers didn't type what the brain told them. I've
meant VPCI_UNPRIVILEGED. I would also be okay with HAS_VPCI_GUEST_SUPPORT
(i.e. not exactly what you've suggested), for example.
Jan
On 07.09.21 15:20, Jan Beulich wrote:
> On 07.09.2021 14:16, Oleksandr Andrushchenko wrote:
>> On 07.09.21 14:49, Jan Beulich wrote:
>>> On 07.09.2021 13:10, Oleksandr Andrushchenko wrote:
>>>> On 07.09.21 13:43, Jan Beulich wrote:
>>>>> On 07.09.2021 12:11, Oleksandr Andrushchenko wrote:
>>>>>> On 06.09.21 17:11, Jan Beulich wrote:
>>>>>>> On 03.09.2021 12:08, Oleksandr Andrushchenko wrote:
>>>>>>>> @@ -593,6 +625,36 @@ static int init_bars(struct pci_dev *pdev)
>>>>>>>> }
>>>>>>>> REGISTER_VPCI_INIT(init_bars, VPCI_PRIORITY_MIDDLE);
>>>>>>>>
>>>>>>>> +int vpci_bar_add_handlers(const struct domain *d, struct pci_dev *pdev)
>>>>>>>> +{
>>>>>>>> + int rc;
>>>>>>>> +
>>>>>>>> + /* Remove previously added registers. */
>>>>>>>> + vpci_remove_device_registers(pdev);
>>>>>>>> +
>>>>>>>> + /* It only makes sense to add registers for hwdom or guest domain. */
>>>>>>>> + if ( d->domain_id >= DOMID_FIRST_RESERVED )
>>>>>>>> + return 0;
>>>>>>>> +
>>>>>>>> + if ( is_hardware_domain(d) )
>>>>>>>> + rc = add_bar_handlers(pdev, true);
>>>>>>>> + else
>>>>>>>> + rc = add_bar_handlers(pdev, false);
>>>>>>> rc = add_bar_handlers(pdev, is_hardware_domain(d));
>>>>>> Indeed, thank you ;)
>>>>>>>> + if ( rc )
>>>>>>>> + gprintk(XENLOG_ERR,
>>>>>>>> + "%pp: failed to add BAR handlers for dom%d\n", &pdev->sbdf,
>>>>>>>> + d->domain_id);
>>>>>>> Please use %pd and correct indentation. Logging the error code might
>>>>>>> also help some in diagnosing issues.
>>>>>> Sure, I'll change it to %pd
>>>>>>> Further I'm not sure this is a
>>>>>>> message we want in release builds
>>>>>> Why not?
>>>>> Excess verbosity: If we have such here, why not elsewhere on error paths?
>>>>> And I hope you agree things will get too verbose if we had such (about)
>>>>> everywhere?
>>>> Agree, will change it to gdprintk
>>>>>>> - perhaps gdprintk()?
>>>>>> I'll change if we decide so
>>>>>>>> + return rc;
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +int vpci_bar_remove_handlers(const struct domain *d, struct pci_dev *pdev)
>>>>>>>> +{
>>>>>>>> + /* Remove previously added registers. */
>>>>>>>> + vpci_remove_device_registers(pdev);
>>>>>>>> + return 0;
>>>>>>>> +}
>>>>>>> Also - in how far is the goal of your work to also make vPCI work for
>>>>>>> x86 DomU-s? If that's not a goal
>>>>>> It is not, unfortunately. The goal is not to break x86 and to enable Arm
>>>>>>> , I'd like to ask that you limit the
>>>>>>> introduction of code that ends up dead there.
>>>>>> What's wrong with this function even if it is a one-liner?
>>>>> The comment is primarily on the earlier function, and then extends to
>>>>> this one.
>>>>>
>>>>>> This way we have a pair vpci_bar_add_handlers/vpci_bar_remove_handlers
>>>>>> and if I understood correctly you suggest vpci_bar_add_handlers/vpci_remove_device_registers?
>>>>>> What would we gain from that, but yet another secret knowledge that in order
>>>>>> to remove BAR handlers one needs to call vpci_remove_device_registers
>>>>>> while I would personally expect to call vpci_bar_add_handlers' counterpart,
>>>>>> vpci_remove_device_registers namely.
>>>>> This is all fine. Yet vpci_bar_{add,remove}_handlers() will, aiui, be
>>>>> dead code on x86.
>>>> vpci_bar_add_handlers will be used by x86 PVH Dom0
>>> Where / when? You add a call from vpci_assign_device(), but besides that
>>> also being dead code on x86 (for now), you can't mean that because
>>> vpci_deassign_device() also calls vpci_bar_remove_handlers().
>> You are right here and both add/remove are not used on x86 PVH Dom0.
>>
>> I am sorry for wasting your time
>>
>>>>> Hence there should be an arrangement allowing the
>>>>> compiler to eliminate this dead code.
>>>> So, the only dead code for x86 here will be vpci_bar_remove_handlers. Yet.
>>>> Because I hope x86 to gain guest support for PVH Dom0 sooner or later.
>>>>
>>>>> Whether that's enclosing these
>>>>> by "#ifdef" or adding early "if(!IS_ENABLED(CONFIG_*))" is secondary.
>>>>> This has a knock-on effect on other functions as you certainly realize:
>>>>> The compiler seeing e.g. the 2nd argument to the add-BARs function
>>>>> always being true allows it to instantiate just a clone of the original
>>>>> function with the respective conditionals removed.
>>>> With the above (e.g. add is going to be used, but not remove) do you
>>>> think it is worth playing with ifdef's to strip that single function and add
>>>> a piece of spaghetti code to save a bit?
>>> No, that I agree wouldn't be worth it.
>>>
>>>> What would that ifdef look like,
>>>> e.g. #ifdef CONFIG_ARM or #ifndef CONFIG_X86 && any other platform, but ARM?
>>> A new setting, preferably; e.g. VCPU_UNPRIVILEGED, to be "select"ed by
>>> architectures as functionality gets enabled.
>> So, as add/remove are only needed for Arm at the moment
>> you suggest I add VCPU_UNPRIVILEGED to Arm's Kconfig to enable
>> compiling vpci_bar_add_handlers/vpci_bar_remove_handlers?
>> To me this is more about vPCI's support for guests, so should we probably call it
>> VPCI_XXX instead? E.g. VPCI_HAS_GUEST_SUPPORT or something which
>> will reflect the nature of the code being gated? VCPU_UNPRIVILEGED sounds
>> like not connected to vpci to me
> And validly so - my fingers didn't type what the brain told them. I've
> meant VPCI_UNPRIVILEGED. I would also be okay with HAS_VPCI_GUEST_SUPPORT
> (i.e. not exactly what you've suggested), for example.
I'll stick to HAS_VPCI_GUEST_SUPPORT as it seems to be more descriptive
> Jan
>
Thank you,
Oleksandr
On Fri, 3 Sep 2021, Oleksandr Andrushchenko wrote:
> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>
> Add relevant vpci register handlers when assigning PCI device to a domain
> and remove those when de-assigning. This allows having different
> handlers for different domains, e.g. hwdom and other guests.
>
> Use stubs for guest domains for now.
>
> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> ---
> xen/drivers/vpci/header.c | 78 +++++++++++++++++++++++++++++++++++----
> xen/drivers/vpci/vpci.c | 4 +-
> xen/include/xen/vpci.h | 4 ++
> 3 files changed, 76 insertions(+), 10 deletions(-)
>
> diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
> index 31bca7a12942..5218b1af247e 100644
> --- a/xen/drivers/vpci/header.c
> +++ b/xen/drivers/vpci/header.c
> @@ -397,6 +397,17 @@ static void bar_write(const struct pci_dev *pdev, unsigned int reg,
> pci_conf_write32(pdev->sbdf, reg, val);
> }
>
> +static void guest_bar_write(const struct pci_dev *pdev, unsigned int reg,
> + uint32_t val, void *data)
> +{
> +}
> +
> +static uint32_t guest_bar_read(const struct pci_dev *pdev, unsigned int reg,
> + void *data)
> +{
> + return 0xffffffff;
> +}
> static void rom_write(const struct pci_dev *pdev, unsigned int reg,
> uint32_t val, void *data)
> {
> @@ -445,14 +456,25 @@ static void rom_write(const struct pci_dev *pdev, unsigned int reg,
> rom->addr = val & PCI_ROM_ADDRESS_MASK;
> }
>
> -static int add_bar_handlers(struct pci_dev *pdev)
> +static void guest_rom_write(const struct pci_dev *pdev, unsigned int reg,
> + uint32_t val, void *data)
> +{
> +}
> +
> +static uint32_t guest_rom_read(const struct pci_dev *pdev, unsigned int reg,
> + void *data)
> +{
> + return 0xffffffff;
> +}
> +
> +static int add_bar_handlers(struct pci_dev *pdev, bool is_hwdom)
> {
> unsigned int i;
> struct vpci_header *header = &pdev->vpci->header;
> struct vpci_bar *bars = header->bars;
> int rc;
>
> - /* Setup a handler for the command register. */
> + /* Setup a handler for the command register: same for hwdom and guests. */
> rc = vpci_add_register(pdev->vpci, vpci_hw_read16, cmd_write, PCI_COMMAND,
> 2, header);
> if ( rc )
> @@ -475,8 +497,13 @@ static int add_bar_handlers(struct pci_dev *pdev)
> rom_reg = PCI_ROM_ADDRESS;
> else
> rom_reg = PCI_ROM_ADDRESS1;
> - rc = vpci_add_register(pdev->vpci, vpci_hw_read32, rom_write,
> - rom_reg, 4, &bars[i]);
> + if ( is_hwdom )
> + rc = vpci_add_register(pdev->vpci, vpci_hw_read32, rom_write,
> + rom_reg, 4, &bars[i]);
> + else
> + rc = vpci_add_register(pdev->vpci,
> + guest_rom_read, guest_rom_write,
> + rom_reg, 4, &bars[i]);
> if ( rc )
> return rc;
> }
> @@ -485,8 +512,13 @@ static int add_bar_handlers(struct pci_dev *pdev)
> uint8_t reg = PCI_BASE_ADDRESS_0 + i * 4;
>
> /* This is either VPCI_BAR_MEM32 or VPCI_BAR_MEM64_{LO|HI}. */
> - rc = vpci_add_register(pdev->vpci, vpci_hw_read32, bar_write, reg,
> - 4, &bars[i]);
> + if ( is_hwdom )
> + rc = vpci_add_register(pdev->vpci, vpci_hw_read32, bar_write,
> + reg, 4, &bars[i]);
> + else
> + rc = vpci_add_register(pdev->vpci,
> + guest_bar_read, guest_bar_write,
> + reg, 4, &bars[i]);
> if ( rc )
> return rc;
> }
> @@ -520,7 +552,7 @@ static int init_bars(struct pci_dev *pdev)
> }
>
> if ( pdev->ignore_bars )
> - return add_bar_handlers(pdev);
> + return add_bar_handlers(pdev, true);
>
> /* Disable memory decoding before sizing. */
> cmd = pci_conf_read16(pdev->sbdf, PCI_COMMAND);
> @@ -582,7 +614,7 @@ static int init_bars(struct pci_dev *pdev)
> PCI_ROM_ADDRESS_ENABLE;
> }
>
> - rc = add_bar_handlers(pdev);
> + rc = add_bar_handlers(pdev, true);
> if ( rc )
> {
> pci_conf_write16(pdev->sbdf, PCI_COMMAND, cmd);
> @@ -593,6 +625,36 @@ static int init_bars(struct pci_dev *pdev)
> }
> REGISTER_VPCI_INIT(init_bars, VPCI_PRIORITY_MIDDLE);
>
> +int vpci_bar_add_handlers(const struct domain *d, struct pci_dev *pdev)
> +{
> + int rc;
> +
> + /* Remove previously added registers. */
> + vpci_remove_device_registers(pdev);
> +
> + /* It only makes sense to add registers for hwdom or guest domain. */
> + if ( d->domain_id >= DOMID_FIRST_RESERVED )
> + return 0;
This check is redundant, isn't it? Because it is already checked by the
caller?
> + if ( is_hardware_domain(d) )
> + rc = add_bar_handlers(pdev, true);
> + else
> + rc = add_bar_handlers(pdev, false);
NIT:
rc = add_bar_handlers(pdev, is_hardware_domain(d));
© 2016 - 2026 Red Hat, Inc.