On Wednesday, January 10, 2024 12:32 AM, Li, Xiaoyao wrote:
> On 1/9/2024 10:53 PM, Wang, Wei W wrote:
> > On Tuesday, January 9, 2024 1:47 PM, Li, Xiaoyao wrote:
> >> On 12/21/2023 9:47 PM, Wang, Wei W wrote:
> >>> On Thursday, December 21, 2023 7:54 PM, Li, Xiaoyao wrote:
> >>>> On 12/21/2023 6:36 PM, Wang, Wei W wrote:
> >>>>> No need to specifically check for KVM_MEMORY_ATTRIBUTE_PRIVATE
> >> there.
> >>>>> I'm suggesting below:
> >>>>>
> >>>>> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c index
> >>>>> 2d9a2455de..63ba74b221 100644
> >>>>> --- a/accel/kvm/kvm-all.c
> >>>>> +++ b/accel/kvm/kvm-all.c
> >>>>> @@ -1375,6 +1375,11 @@ static int
> kvm_set_memory_attributes(hwaddr
> >>>> start, hwaddr size, uint64_t attr)
> >>>>> struct kvm_memory_attributes attrs;
> >>>>> int r;
> >>>>>
> >>>>> + if ((attr & kvm_supported_memory_attributes) != attr) {
> >>>>> + error_report("KVM doesn't support memory attr %lx\n", attr);
> >>>>> + return -EINVAL;
> >>>>> + }
> >>>>
> >>>> In the case of setting a range of memory to shared while KVM
> >>>> doesn't support private memory. Above check doesn't work. and
> >>>> following IOCTL
> >> fails.
> >>>
> >>> SHARED attribute uses the value 0, which indicates it's always supported,
> no?
> >>> For the implementation, can you find in the KVM side where the ioctl
> >>> would get failed in that case?
> >>
> >> I'm worrying about the future case, that KVM supports other memory
> >> attribute than shared/private. For example, KVM supports RWX bits
> >> (bit 0
> >> - 2) but not shared/private bit.
> >
> > What's the exact issue?
> > +#define KVM_MEMORY_ATTRIBUTE_READ (1ULL << 2)
> > +#define KVM_MEMORY_ATTRIBUTE_WRITE (1ULL << 1)
> > +#define KVM_MEMORY_ATTRIBUTE_EXE (1ULL << 0)
> >
> > They are checked via
> > "if ((attr & kvm_supported_memory_attributes) != attr)" shared above
> > in kvm_set_memory_attributes.
> > In the case you described, kvm_supported_memory_attributes will be 0x7.
> > Anything unexpected?
>
> Sorry that I thought for wrong case.
>
> It doesn't work on the case that KVM doesn't support memory_attribute, e.g.,
> an old KVM. In this case, 'kvm_supported_memory_attributes' is 0, and 'attr' is
> 0 as well.
How is this different in your existing implementation?
The official way of defining a feature is to take a bit (based on the first feature,
*_PRIVATE, defined). Using 0 as an attr is a bit magic and it passes all the "&" based check.
But using it for *_SHARED looks fine to me as semantically memory can always be shared
and the ioctl will return with -ENOTTY anyway in your mentioned case.
