linux-kernel - Re: [PATCH v11 22/26] virt: gunyah: Add proxy-scheduled vCPUs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <274ad221-f397-b634-5742-fe6c9cb18843@quicinc.com>
Date:   Mon, 17 Apr 2023 15:41:58 -0700
From:   Elliot Berman <quic_eberman@...cinc.com>
To:     Alex Elder <elder@...aro.org>,
        Srinivas Kandagatla <srinivas.kandagatla@...aro.org>,
        Prakruthi Deepak Heragu <quic_pheragu@...cinc.com>,
        Jonathan Corbet <corbet@....net>,
        Catalin Marinas <catalin.marinas@....com>,
        Will Deacon <will@...nel.org>
CC:     Murali Nalajala <quic_mnalajal@...cinc.com>,
        Trilok Soni <quic_tsoni@...cinc.com>,
        Srivatsa Vaddagiri <quic_svaddagi@...cinc.com>,
        Carl van Schaik <quic_cvanscha@...cinc.com>,
        Dmitry Baryshkov <dmitry.baryshkov@...aro.org>,
        Bjorn Andersson <andersson@...nel.org>,
        "Konrad Dybcio" <konrad.dybcio@...aro.org>,
        Arnd Bergmann <arnd@...db.de>,
        "Greg Kroah-Hartman" <gregkh@...uxfoundation.org>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Bagas Sanjaya <bagasdotme@...il.com>,
        Andy Gross <agross@...nel.org>,
        Jassi Brar <jassisinghbrar@...il.com>,
        <linux-arm-msm@...r.kernel.org>, <devicetree@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <linux-doc@...r.kernel.org>,
        <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH v11 22/26] virt: gunyah: Add proxy-scheduled vCPUs

On 3/31/2023 7:27 AM, Alex Elder wrote:
> On 3/3/23 7:06 PM, Elliot Berman wrote:

[snip]

>> diff --git a/include/uapi/linux/gunyah.h b/include/uapi/linux/gunyah.h
>> index caeb3b3a3e9a..e52265fa5715 100644
>> --- a/include/uapi/linux/gunyah.h
>> +++ b/include/uapi/linux/gunyah.h
>> @@ -62,8 +62,32 @@ struct gh_vm_dtb_config {
>>   #define GH_VM_START        _IO(GH_IOCTL_TYPE, 0x3)
>> +/**
>> + * GH_FN_VCPU - create a vCPU instance to control a vCPU
>> + *
>> + * gh_fn_desc is filled with &struct gh_fn_vcpu_arg
>> + *
>> + * The vcpu type will register with the VM Manager to expect to control
>> + * vCPU number `vcpu_id`. It returns a file descriptor allowing 
>> interaction with
>> + * the vCPU. See the Gunyah vCPU API description sections for 
>> interacting with
>> + * the Gunyah vCPU file descriptors.
>> + *
>> + * Return: file descriptor to manipulate the vcpu. See GH_VCPU_* ioctls
>> + */
>> +#define GH_FN_VCPU         1
> 
> I think you should define GH_VN_VCPU, GN_FN_IRQFD, and GN_FN_IOEVENTFD
> in an enumerated type.  Each has a type associated with it, and you can
> add the explanation for the function in the kernel-doc comments above
> thosse type definitions.
> 

I'd like to enumify the GH_FN_* macros, but one challenge I'm facing is 
that it breaks the module alias implementation in patch 19.

MODULE_ALIAS("ghfunc:"__stringify(_type))

When the GH_FN_* are regular preprocessor macros backed by an integer, 
the preprocessor will make the module alias ghfunc:0 (or ghfunc:1, etc). 
This works well because I can do

request_module("ghfunc:%d", type);

If the function hasn't been registered and then gunyah_vcpu.ko gets 
loaded automatically.

With enum, compiler knows the value of GH_FN_VCPU and preprocessor will 
make the module alias like ghfunc:GH_FN_VCPU.

[snip]

>> +
>> +/*
>> + * Gunyah presently sends max 4 bytes of exit_reason.
>> + * If that changes, this macro can be safely increased without breaking
>> + * userspace so long as struct gh_vcpu_run < PAGE_SIZE.
> 
> Is PAGE_SIZE allowed to be anything other than 4096 bytes?  Do you
> expect this driver to work properly if the page size were configured
> to be 16384 bytes?  In other words, is this a Gunyah constant, or
> is it *really* the page size configured for Linux?
> 

Our implementations are only doing 4096 bytes. I expect the driver to 
work properly when using 16k pages. This really is a Linux page. It's a 
reflection of the alloc_page in gh_vcpu_bind().

The exit reason is copied from hypervisor into field accessible by 
userspace directly. Gunyah makes the exit reason size dynamic -- there's 
no architectural limitation preventing the exit reason from being a 
string or some lengthy data.

As I was writing this response, I realized that I should be able to make 
this a zero-length array and ensure that reason[] doesn't overflow 
PAGE_SIZE...

The comment was trying to explain that Linux itself imposes a limitation 
on the maximum exit reason size. If we need to support longer exit 
reason, we're OK to do so long as the total size doesn't overrun 
PAGE_SIZE. There aren't any plans to need longer exit reasons than the 8 
bytes mentioned today.

Thanks,
Elliot