Re: KVM: PCIPT: direct mmio

[Avi Kivity <avi@qumranet.com>]

Ben-Ami Yassour wrote:
>
>>> +    if (len > 0) {
>>> +        ptr = mmap(NULL, len, prot, MAP_ANONYMOUS | MAP_SHARED, -1, 
>>> 0);
>>> +        if (ptr == MAP_FAILED) {
>>> +            fprintf(stderr, "create_userspace_phys_mem: %s",
>>> +                strerror(errno));
>>> +            return 0;
>>> +        }
>>
>> You're using 'len == 0' here to change the semantics of the function.
>> It would be better to have two different APIs (perhaps sharing some of
>> the implementation by calling a helper).
>>
>
> Actually, this is a fix of a bug that is probably exposed only by the 
> direct mmio code.
> Here is the problem (in the existing code):
> kvm_destroy_phys_mem calls kvm_create_phys_mem(kvm, phys_start, 0, 0, 0);
> kvm_create_phys_mem calls kvm_create_userspace_phys_mem which is calling
>
> mmap(NULL, len, prot, MAP_ANONYMOUS | MAP_SHARED, -1, 0);
>         if (ptr == MAP_FAILED)
>
> now, if len = 0 it fails.
>

What's the point of a zero length memslot?  I seem to remember this 
conversation in the past.

> We could have sent is as a separate patch,
> and we could have made more changes to fix it differently,
> for example, not to let kvm_destroy_phys_mem call kvm_create_phys_mem
> (which seems strange in general...).

Yeah.  Perhaps a common helper.

> We wanted to minimize the amount of changes that we make, so we choose 
> this option.
>
> What do you recommend?

Send patches that fix the current broken code and prepare the way for 
the real changes.

This way its easy to review both the fixes to the existing code and the 
new functionality.

-- 
error compiling committee.c: too many arguments to function