Messages in this thread | | | Subject | Re: [RFC PATCH 4/6] virtio: Various updates to xen-virtio DMA ops layer | From | Oleksandr <> | Date | Tue, 19 Apr 2022 10:07:14 +0300 |
| |
Hello Stefano, Juergen
On 19.04.22 09:58, Juergen Gross wrote: > On 18.04.22 21:11, Stefano Stabellini wrote: >> On Sun, 17 Apr 2022, Oleksandr wrote: >>> On 16.04.22 01:02, Stefano Stabellini wrote: >>>> On Thu, 14 Apr 2022, Oleksandr Tyshchenko wrote: >>>>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >>>>> >>>>> In the context of current patch do the following: >>>>> 1. Update code to support virtio-mmio devices >>>>> 2. Introduce struct xen_virtio_data and account passed virtio devices >>>>> (using list) as we need to store some per-device data >>>>> 3. Add multi-page support for xen_virtio_dma_map(unmap)_page >>>>> callbacks >>>>> 4. Harden code against malicious backend >>>>> 5. Change to use alloc_pages_exact() instead of __get_free_pages() >>>>> 6. Introduce locking scheme to protect mappings (I am not 100% sure >>>>> whether per-device lock is really needed) >>>>> 7. Handle virtio device's DMA mask >>>>> 8. Retrieve the ID of backend domain from DT for virtio-mmio device >>>>> instead of hardcoding it. >>>>> >>>>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >>>>> --- >>>>> arch/arm/xen/enlighten.c | 11 +++ >>>>> drivers/xen/Kconfig | 2 +- >>>>> drivers/xen/xen-virtio.c | 200 >>>>> ++++++++++++++++++++++++++++++++++++++++++----- >>>>> include/xen/xen-ops.h | 5 ++ >>>>> 4 files changed, 196 insertions(+), 22 deletions(-) >>>>> >>>>> diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c >>>>> index ec5b082..870d92f 100644 >>>>> --- a/arch/arm/xen/enlighten.c >>>>> +++ b/arch/arm/xen/enlighten.c >>>>> @@ -409,6 +409,17 @@ int __init arch_xen_unpopulated_init(struct >>>>> resource >>>>> **res) >>>>> } >>>>> #endif >>>>> +#ifdef CONFIG_ARCH_HAS_RESTRICTED_VIRTIO_MEMORY_ACCESS >>>>> +int arch_has_restricted_virtio_memory_access(void) >>>>> +{ >>>>> + if (IS_ENABLED(CONFIG_XEN_HVM_VIRTIO_GRANT) && xen_hvm_domain()) >>>>> + return 1; >>>> Instead of xen_hvm_domain(), you can just use xen_domain(). Also there >>>> is no need for the #ifdef >>>> CONFIG_ARCH_HAS_RESTRICTED_VIRTIO_MEMORY_ACCESS, given that: >>>> >>>> CONFIG_XEN_HVM_VIRTIO_GRANT depends on XEN_VIRTIO which selects >>>> ARCH_HAS_RESTRICTED_VIRTIO_MEMORY_ACCESS >>> >>> >>> Yes, but please see my comments in commit #2 regarding >>> CONFIG_XEN_HVM_VIRTIO_GRANT option and >>> arch_has_restricted_virtio_memory_access() on Arm. >>> >>> I propose to have the following on Arm: >>> >>> int arch_has_restricted_virtio_memory_access(void) >>> { >>> return xen_has_restricted_virtio_memory_access(); >>> } >>> >>> >>> where common xen.h contain a helper to be used by both Arm and x86: >>> >>> static inline int xen_has_restricted_virtio_memory_access(void) >>> { >>> if (IS_ENABLED(CONFIG_XEN_VIRTIO) && (xen_pv_domain() || >>> xen_hvm_domain())) >>> return 1; >>> >>> return 0; >>> } >>> >>> >>> But I would be happy with what you propose as well. >> >> As I wrote in the previous reply, I also prefer to share the code >> between x86 and ARM, and I think it could look like: >> >> int arch_has_restricted_virtio_memory_access(void) >> { >> return xen_has_restricted_virtio_memory_access(); >> } >> [...] >> static inline int xen_has_restricted_virtio_memory_access(void) >> { >> return (IS_ENABLED(CONFIG_XEN_VIRTIO) && xen_domain()); >> } >> >> But let's check with Juergen and Boris.
for the record, it is already clarified in commit #2, I will use this variant.
>> >> >> >>>>> + return 0; >>>>> +} >>>>> +EXPORT_SYMBOL_GPL(arch_has_restricted_virtio_memory_access); >>>>> +#endif >>>>> + >>>>> static void __init xen_dt_guest_init(void) >>>>> { >>>>> struct device_node *xen_node; >>>>> diff --git a/drivers/xen/Kconfig b/drivers/xen/Kconfig >>>>> index fc61f7a..56afe6a 100644 >>>>> --- a/drivers/xen/Kconfig >>>>> +++ b/drivers/xen/Kconfig >>>>> @@ -347,7 +347,7 @@ config XEN_VIRTIO >>>>> config XEN_HVM_VIRTIO_GRANT >>>>> bool "Require virtio for fully virtualized guests to use grant >>>>> mappings" >>>>> - depends on XEN_VIRTIO && X86_64 >>>>> + depends on XEN_VIRTIO && (X86_64 || ARM || ARM64) >>>> you can remove the architectural dependencies >>> >>> >>> According to the conversation in commit #2 we are considering just a >>> single >>> XEN_VIRTIO option, but it is going to has the >>> same architectural dependencies: (X86_64 || ARM || ARM64) >>> >>> By removing the architectural dependencies here, we will leave also >>> X86_32 >>> covered (neither XEN_HVM_VIRTIO_GRANT nor XEN_PV_VIRTIO covered it). >>> I don't >>> know whether it is ok or not. >>> >>> Shall I remove dependencies anyway? >> >> No, good point. I don't know about X86_32. This is another detail where >> Juergen or Boris should comment. > > X86_32 should in theory work (it is HVM/PVH only, as PV 32-bit guests > are no > longer supported).
ok, thank you for confirming. I will drop architectural dependencies then.
> > > > Juergen
-- Regards,
Oleksandr Tyshchenko
| |