Re: [Qemu-devel] [PATCH-for-4.1 v3 5/6] virtio-balloon: Rework pbp tracking data

[David Hildenbrand]

On 23.07.19 04:54, David Gibson wrote:
> On Mon, Jul 22, 2019 at 03:41:07PM +0200, David Hildenbrand wrote:
>> Using the address of a RAMBlock to test for a matching pbp is not really
>> safe. Instead, let's use the guest physical address of the base page
>> along with the page size (via the number of subpages).
>>
>> Also, let's allocate the bitmap separately. This makes the code
>> easier to read and maintain - we can reuse bitmap_new().
>>
>> Prepare the code to move the PBP out of the device.
>>
>> Fixes: ed48c59875b6 ("virtio-balloon: Safely handle BALLOON_PAGE_SIZE <
>>                      host page size")
>> Fixes: b27b32391404 ("virtio-balloon: Fix possible guest memory corruption
>>                      with inflates & deflates")
>> Cc: address@hidden #v4.0.0
>> Signed-off-by: David Hildenbrand <address@hidden>
>> ---
>>  hw/virtio/virtio-balloon.c | 69 +++++++++++++++++++++++++-------------
>>  1 file changed, 46 insertions(+), 23 deletions(-)
>>
>> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
>> index f206cc8bf7..40d493a31a 100644
>> --- a/hw/virtio/virtio-balloon.c
>> +++ b/hw/virtio/virtio-balloon.c
>> @@ -35,16 +35,44 @@
>>  #define BALLOON_PAGE_SIZE  (1 << VIRTIO_BALLOON_PFN_SHIFT)
>>  
>>  struct PartiallyBalloonedPage {
>> -    RAMBlock *rb;
>> -    ram_addr_t base;
>> -    unsigned long bitmap[];
>> +    ram_addr_t base_gpa;
>> +    long subpages;
>> +    unsigned long *bitmap;
>>  };
>>  
>> +static void virtio_balloon_pbp_free(PartiallyBalloonedPage *pbp)
>> +{
>> +    if (!pbp) {
>> +        return;
>> +    }
>> +    g_free(pbp->bitmap);
>> +    g_free(pbp);
>> +}
>> +
>> +static PartiallyBalloonedPage *virtio_balloon_pbp_alloc(ram_addr_t base_gpa,
>> +                                                        long subpages)
>> +{
>> +    PartiallyBalloonedPage *pbp = g_new0(PartiallyBalloonedPage, 1);
>> +
>> +    pbp->base_gpa = base_gpa;
>> +    pbp->subpages = subpages;
>> +    pbp->bitmap = bitmap_new(subpages);
>> +
>> +    return pbp;
>> +}
>> +
>> +static bool virtio_balloon_pbp_matches(PartiallyBalloonedPage *pbp,
>> +                                       ram_addr_t base_gpa, long subpages)
>> +{
>> +    return pbp->subpages == subpages && pbp->base_gpa == base_gpa;
> 
> I think the test on subpages is pointless here.  You've (reasonably)
> rejected handling the edge case of a ramblock being removed and
> replugged - but the only thing this handles is an edge case of that
> edge case.

I think we have to leave it in place until patch #6. We could remove it
after Patch #6 - replug cannot happen while processing the inflation
requests.

-- 

Thanks,

David / dhildenb