On 2024/8/1 19:06, Roger Pau Monné wrote:
> On Mon, Jul 08, 2024 at 07:41:21PM +0800, Jiqian Chen wrote:
>> Some type of domains don't have PIRQs, like PVH, it doesn't do
>> PHYSDEVOP_map_pirq for each gsi. When passthrough a device
>> to guest base on PVH dom0, callstack
>> pci_add_dm_done->XEN_DOMCTL_irq_permission will fail at function
>> domain_pirq_to_irq, because PVH has no mapping of gsi, pirq and
>> irq on Xen side.
>> What's more, current hypercall XEN_DOMCTL_irq_permission requires
>> passing in pirq to set the access of irq, it is not suitable for
>> dom0 that doesn't have PIRQs.
>>
>> So, add a new hypercall XEN_DOMCTL_gsi_permission to grant/deny
>> the permission of irq(translate from x86 gsi) to dumU when dom0
> ^ missing space, and s/translate/translated/
>
>> has no PIRQs.
>>
>> Signed-off-by: Jiqian Chen <jiqian.c...@amd.com>
>> Signed-off-by: Huang Rui <ray.hu...@amd.com>
>> Signed-off-by: Jiqian Chen <jiqian.c...@amd.com>
>> ---
>> CC: Daniel P . Smith <dpsm...@apertussolutions.com>
>> Remaining comment @Daniel P . Smith:
>> + ret = -EPERM;
>> + if ( !irq_access_permitted(currd, irq) ||
>> + xsm_irq_permission(XSM_HOOK, d, irq, access_flag) )
>> + goto gsi_permission_out;
>> Is it okay to issue the XSM check using the translated value,
>> not the one that was originally passed into the hypercall?
>
> FWIW, I don't see the GSI -> IRQ translation much different from the
> pIRQ -> IRQ translation done by pirq_access_permitted(), which is also
> ahead of the xsm check.
>
>> ---
>> xen/arch/x86/domctl.c | 32 ++++++++++++++++++++++++++++++
>> xen/arch/x86/include/asm/io_apic.h | 2 ++
>> xen/arch/x86/io_apic.c | 17 ++++++++++++++++
>> xen/arch/x86/mpparse.c | 5 ++---
>> xen/include/public/domctl.h | 9 +++++++++
>> xen/xsm/flask/hooks.c | 1 +
>> 6 files changed, 63 insertions(+), 3 deletions(-)
>>
>> diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
>> index 9190e11faaa3..4e9e4c4cfed3 100644
>> --- a/xen/arch/x86/domctl.c
>> +++ b/xen/arch/x86/domctl.c
>> @@ -36,6 +36,7 @@
>> #include <asm/xstate.h>
>> #include <asm/psr.h>
>> #include <asm/cpu-policy.h>
>> +#include <asm/io_apic.h>
>>
>> static int update_domain_cpu_policy(struct domain *d,
>> xen_domctl_cpu_policy_t *xdpc)
>> @@ -237,6 +238,37 @@ long arch_do_domctl(
>> break;
>> }
>>
>> + case XEN_DOMCTL_gsi_permission:
>> + {
>> + int irq;
>> + unsigned int gsi = domctl->u.gsi_permission.gsi;
>> + uint8_t access_flag = domctl->u.gsi_permission.access_flag;
>> +
>> + /* Check all bits and pads are zero except lowest bit */
>> + ret = -EINVAL;
>> + if ( access_flag & ( ~XEN_DOMCTL_GSI_PERMISSION_MASK ) )
> ^ unneeded parentheses and spaces.
>> + goto gsi_permission_out;
>> + for ( i = 0; i < ARRAY_SIZE(domctl->u.gsi_permission.pad); ++i )
>> + if ( domctl->u.gsi_permission.pad[i] )
>> + goto gsi_permission_out;
>> +
>> + if ( gsi > highest_gsi() || (irq = gsi_2_irq(gsi)) <= 0 )
>
> FWIW, I would place the gsi > highest_gsi() check inside gsi_2_irq().
> There's no reason to open-code it here, and it could help other
> users of gsi_2_irq(). The error code could also be ERANGE here
> instead of EINVAL IMO.
>
>> + goto gsi_permission_out;
>> +
>> + ret = -EPERM;
>> + if ( !irq_access_permitted(currd, irq) ||
>> + xsm_irq_permission(XSM_HOOK, d, irq, access_flag) )
>> + goto gsi_permission_out;
>> +
>> + if ( access_flag )
>> + ret = irq_permit_access(d, irq);
>> + else
>> + ret = irq_deny_access(d, irq);
>> +
>> + gsi_permission_out:
>> + break;
>
> Why do you need a label when it just contains a break? Instead of the
> goto gsi_permission_out just use break directly.
>
>> + }
>> +
>> case XEN_DOMCTL_getpageframeinfo3:
>> {
>> unsigned int num = domctl->u.getpageframeinfo3.num;
>> diff --git a/xen/arch/x86/include/asm/io_apic.h
>> b/xen/arch/x86/include/asm/io_apic.h
>> index 78268ea8f666..7e86d8337758 100644
>> --- a/xen/arch/x86/include/asm/io_apic.h
>> +++ b/xen/arch/x86/include/asm/io_apic.h
>> @@ -213,5 +213,7 @@ unsigned highest_gsi(void);
>>
>> int ioapic_guest_read( unsigned long physbase, unsigned int reg, u32 *pval);
>> int ioapic_guest_write(unsigned long physbase, unsigned int reg, u32 val);
>> +int mp_find_ioapic(int gsi);
>> +int gsi_2_irq(int gsi);
>>
>> #endif
>> diff --git a/xen/arch/x86/io_apic.c b/xen/arch/x86/io_apic.c
>> index d2a313c4ac72..5968c8055671 100644
>> --- a/xen/arch/x86/io_apic.c
>> +++ b/xen/arch/x86/io_apic.c
>> @@ -955,6 +955,23 @@ static int pin_2_irq(int idx, int apic, int pin)
>> return irq;
>> }
>>
>> +int gsi_2_irq(int gsi)
>
> unsigned int for gsi.
>
>> +{
>> + int ioapic, pin, irq;
>
> pin would better be unsigned int also.
>
>> +
>> + ioapic = mp_find_ioapic(gsi);
>> + if ( ioapic < 0 )
>> + return -EINVAL;
>> +
>> + pin = gsi - io_apic_gsi_base(ioapic);
>> +
>> + irq = apic_pin_2_gsi_irq(ioapic, pin);
>> + if ( irq <= 0 )
>> + return -EINVAL;
>> +
>> + return irq;
>> +}
>> +
>> static inline int IO_APIC_irq_trigger(int irq)
>> {
>> int apic, idx, pin;
>> diff --git a/xen/arch/x86/mpparse.c b/xen/arch/x86/mpparse.c
>> index d8ccab2449c6..7786a3337760 100644
>> --- a/xen/arch/x86/mpparse.c
>> +++ b/xen/arch/x86/mpparse.c
>> @@ -841,8 +841,7 @@ static struct mp_ioapic_routing {
>> } mp_ioapic_routing[MAX_IO_APICS];
>>
>>
>> -static int mp_find_ioapic (
>> - int gsi)
>> +int mp_find_ioapic(int gsi)
>
> If you are changing this, you might as well make the gsi parameter
> unsigned int.
Thanks, I will change codes according above comments in next version.
>
>> {
>> unsigned int i;
>>
>> @@ -914,7 +913,7 @@ void __init mp_register_ioapic (
>> return;
>> }
>>
>> -unsigned __init highest_gsi(void)
>> +unsigned highest_gsi(void)
>> {
>> unsigned x, res = 0;
>> for (x = 0; x < nr_ioapics; x++)
>> diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
>> index 2a49fe46ce25..877e35ab1376 100644
>> --- a/xen/include/public/domctl.h
>> +++ b/xen/include/public/domctl.h
>> @@ -464,6 +464,13 @@ struct xen_domctl_irq_permission {
>> uint8_t pad[3];
>> };
>>
>> +/* XEN_DOMCTL_gsi_permission */
>> +struct xen_domctl_gsi_permission {
>> + uint32_t gsi;
>> +#define XEN_DOMCTL_GSI_PERMISSION_MASK 1
>
> IMO this would be better named GRANT or similar, maybe something like:
>
> /* Low bit used to signal grant/revoke action. */
> #define XEN_DOMCTL_GSI_REVOKE 0
> #define XEN_DOMCTL_GSI_GRANT 1
>
>> + uint8_t access_flag; /* flag to specify enable/disable of x86 gsi
>> access */
>> + uint8_t pad[3];
>
> We might as well declare the flags field as uint32_t and avoid the
> padding field.
So, should this struct be like below? Then I just need to check whether
everything except the lowest bit is 0.
struct xen_domctl_gsi_permission {
uint32_t gsi;
/* Lowest bit used to signal grant/revoke action. */
#define XEN_DOMCTL_GSI_REVOKE 0
#define XEN_DOMCTL_GSI_GRANT 1
#define XEN_DOMCTL_GSI_PERMISSION_MASK 1
uint32_t access_flag; /* flag to specify enable/disable of x86 gsi
access */
};
>
> Thanks, Roger.
--
Best regards,
Jiqian Chen.
